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

Dispatch Routing from yaml #3136

Merged
merged 23 commits into from
Aug 22, 2019
Merged

Conversation

whoarethebritons
Copy link
Contributor

@whoarethebritons whoarethebritons commented Aug 9, 2019

AdminServer accepts dispatchRules in the format described here sent from the appscale-tools. The rules are modified to be "nginx friendly" by replacing * with .* and filling out the service name as gae_project_service_version. These rules are then put into zookeeper and picked up during the duty cycle where it regenerates routing config. They are used in the new template file application_nginx.erb

Known Issues:

  • Currently login URL creation will get redirected to the service's port even if you just came from the default port. (I am working on a branch to resolve this)
  • The AdminServer will respond instantly saying the dispatchRules were applied although it might take until the next duty cycle for them to be applied.
  • It seems like the changes to the python AppServer keep master behavior but we currently do not have anything comparable for the Java AppServer since url patterns that are supposed to be secured do not appear to be set in nginx in master. ref: Java Secure URLs #3137

Build:
http://ocd.appscale.net:8080/job/Daily%20Build/6607/

Copy link
Member

@cdonati cdonati left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I love the simplification in nginx config file generation.

AdminServer/appscale/admin/__init__.py Outdated Show resolved Hide resolved
AdminServer/appscale/admin/utils.py Outdated Show resolved Hide resolved
project_id=name,
services=service_ids)

dispatch_node = '/'.join(['/appscale', 'projects', name, 'dispatch'])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
dispatch_node = '/'.join(['/appscale', 'projects', name, 'dispatch'])
dispatch_node = '/appscale/projects/{}/dispatch'.format(name)

'field(s): [{}]'.format(', '.join(supported_fields)))
raise CustomHTTPError(HTTPCodes.BAD_REQUEST, message=message)

project_node = '/'.join(['/appscale', 'projects', name])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
project_node = '/'.join(['/appscale', 'projects', name])
project_node = '/appscale/projects/{}'.format(name)

raise CustomHTTPError(HTTPCodes.BAD_REQUEST, message=message)

project_node = '/'.join(['/appscale', 'projects', name])
services_node = '/'.join([project_node, 'services'])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
services_node = '/'.join([project_node, 'services'])
services_node = '{}/services'.format(project_node)

AdminServer/appscale/admin/constants.py Show resolved Hide resolved
raise InvalidDispatchConfiguration(
'Invalid host pattern {}'.format(domain))
matcher = _URL_IP_V4_ADDR_RE.match(domain)
if matcher and sum(1 for x in matcher.groups() if int(x) <= 255) == 4:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we decide to keep this regex match:

Suggested change
if matcher and sum(1 for x in matcher.groups() if int(x) <= 255) == 4:
if matcher and all(int(x) <= 255 for x in matcher.groups()):

If we use a more accurate regex, this becomes:

    if matcher is not None:

@scragraham
Copy link
Contributor

template of nginx config is: 👍

@cdonati cdonati merged commit 25d07ce into AppScale:master Aug 22, 2019
@scragraham scragraham added this to the 3.8.0 milestone Aug 22, 2019
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 this pull request may close these issues.

3 participants