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

Regression in launch validation causing ingress to exit if --{tcp,udp}-services-configmap specified but doesn't exist #443

Closed
chancez opened this issue Mar 14, 2017 · 4 comments · Fixed by #446
Labels

Comments

@chancez
Copy link
Member

chancez commented Mar 14, 2017

In #432, additional validation was added, which ensures the configmaps specified via configuration exist, before allowing the ingress controller to start.

This might make sense for the --configmap option, but definitely not for --tcp-services-configmap and --udp-services-configmap which are supplemental configuration files, but not necessarily required configuration files.

Previously, ingress would log warnings stating these configmaps don't exist every once in a while, but now ingress just fatally exits at startup if the tcp or udp configmaps do not exist. I believe the validation should only be for --configmap, and the existing behavior is restored.

I can describe a use-case which I think justifies the old behavior:

  • As an admin, I would like to configure a TCP services configuration via configmap, but I do not always want to be required to update my ingress deployment in order to do so, as I want to continue receiving traffic without having to worry about rolling out a new pod for this.
  • As an admin I want to be able to arbitrary remove my tcp-services config as a means of disabling any of those configuration options I did have, but I shouldn't be required to empty the configmap's values, deleting it should work equally as well IMO.

CC @gianrubio and @aledbf

@chancez chancez changed the title Regression in launch validation Regression in launch validation causing ingress to exit if --{tcp,udp}-services-configmap specified but doesn't exist Mar 14, 2017
@chancez
Copy link
Member Author

chancez commented Mar 14, 2017

I almost think all validation added in #432 is questionable now that I think about it.

Why validate any of these resource exist, if the ingress controller could simply watch for these missing resources to show up exist.

Including the watch namespace flag, it could easily watch for new namespaces, and once the --watch-namespace value shows up, the ingress could then begin monitoring for ingress rules, endpoints, and services in that namespace.

So I ask, is any validation of those flags necessary, except for perhaps ensuring they're valid formats? The only one I can think that makes sense to always validate is --configmap. But even then, perhaps it should just wait for that configmap to exist before doing anything else, rather than error and exit when it doesn't exist.

Thoughts?

@chancez
Copy link
Member Author

chancez commented Mar 14, 2017

For what its worth, I don't necessarily think what I describe for --watch-namespace is what should be done, but I think it's worth mentioning as a possible option. I'd like to know more about what determines whether or not a controlling flag/config option is validated and required to exist, etc.

@aledbf aledbf added the nginx label Mar 14, 2017
@gianrubio
Copy link
Contributor

@chancez first I'm sorry for the inconvenience, I know how we felt when a change broke the code, this was not my intention

I previously talked with @aledbf and this was not planned to be available in this release, so we'll get feedback from other user if this options is useful and if aren't breaking ingress.

The ideia of this validation #432 was to check if you have all the configmaps provided as arguments, take a look on #441 where I fixed the code to just check if the configmap exist when you provide the arguments.

I did this PR after "fighting" with my ingress for 1 hour and just realised that my provided configmap doesn't exist. To me make sense to check this parameters if I'm providing.

I'm open for any change, let's heard what the other users think about this.

ps: I'm rollbacking this to avoid this release break other users

@chancez
Copy link
Member Author

chancez commented Mar 15, 2017

@gianrubio Yeah, I'm actually not concerned too much about breakage, but I do care a lot about preferable behavior. I definitely see the point of the validation, I just want to make sure it's being done for the correct stuff :)

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

Successfully merging a pull request may close this issue.

3 participants