argument service_control_network_fail_open is unclear

[mark-00]

The current definition of the arrgument service_control_network_fail_open is unclear

    parser.add_argument('--service_control_network_fail_open',
        default=True, action='store_true', help='''
        In case of network failures when connecting to Google service control,
        the requests will be allowed if this flag is on. Default is on.
        ''')

Default is not on: if this parameter is not specified traffic is blocked on failure connecting to google service controle
"default=True" makes no sense with action='store_true'. it will create its own default in this case False. This creates a lot of confusion.

I propose to change the code to

    parser.add_argument('--service_control_network_fail_open',
        action='store_true', help='''
        In case of network failures when connecting to Google service control,
        the requests will be allowed if this flag is set.
        ''')

[qiwzhang]

Actually, action="sture_true" doesn't set the default value to True. There are many flags in ESP using it, their default values are False. This behaviors is not as documented here, we have not figured it out why.

For --service_control_network_fail_open, we have to set default value to True.

[mark-00]

Correct action="store_true" sets the default value to False. And I think this is correct behavoir.
using the flag --service_control_network_fail_open set the value to true and this results in fail open.
without the flag the value is false and connections are not allowed when the control network fails.

What you describe is setting the default value (when the parameter is not specified) to true and the value when the parameter is specified also to true. This makes no sense.

from https://docs.python.org/3/library/argparse.html

'store_true' and 'store_false' - These are special cases of 'store_const' used for storing the values
True and False respectively. 
In addition, they create default values of False and True respectively.

So no need to specify defaults when using store_true and notice how they use the opposite default of what they are storing

[qiwzhang]

I see your point now; If you just add the flag --service_control_network_fail_open, its value will be True from store_true, it means "fail_open". By common sense, if it is not specified, it should do the opposite, it should have been "fail_close". But since we have default=True, it will cause that even it is not specified, it is "fail_open" already. So this flag is not very useful. Unless you specify --service_control_network_fail_open=False, the result is different.

I will suggest to add a new flag: --service_control_network_policy=[open|close], default is open. this is more clear.
This will be added in ESPv2.

[mark-00]

I can not specify False for the parameter

docker run gcr.io/endpoints-release/endpoints-runtime-secure:1.51  --service_control_network_fail_open=False
usage: start_esp [-h] [-k SERVICE_ACCOUNT_KEY] [-s SERVICE] [-v VERSION]
                 [-n NGINX_CONFIG] [-p HTTP_PORT] [-P HTTP2_PORT]
                 [-S SSL_PORT] [-N STATUS_PORT] [-a BACKEND] [-t]
....
start_esp: error: argument --service_control_network_fail_open: ignored explicit argument 'False'

I'm ok with your proposal for v2. But I would make the default false because this is also the curren default. And is more secure: fail safe

Can the code for v1 be fixed in the following way. It will not change the behavior it will only make clear what the behavior is.

    parser.add_argument('--service_control_network_fail_open',
        action='store_true', help='''
        In case of network failures when connecting to Google service control,
        the requests will be allowed if this flag is set.
        ''')

[qiwzhang]

Due to Google availability requirement, we have to set network_fail_open to true for default ( for most users). Each individual deployment can decide to alter it. e.g. for your case, you prefer to network_fail_close for security.

But as you pointed it out: --service_control_network_fail_open=False doesn't work. We need to come up a way for users to turn it off.

I will implement GoogleCloudPlatform/esp-v2#345 in v1 too. In this repo.

[qiwzhang]

#818