Skip to content
This repository has been archived by the owner on Dec 4, 2024. It is now read-only.

Fix return type of get_app_port_mappings #494

Merged
merged 2 commits into from
Sep 15, 2017
Merged

Conversation

drewkerrigan
Copy link
Contributor

This change addresses a bug introduced marathon-lb version 1.9.0 which affects apps using USER networking mode with containers as well as on DC/OS versions 1.9 and lower. Specifically apps with empty portMappings.

The encountered error looks similar to this:

File "/marathon-lb/utils.py", line 376, in get_port_mapping_ports
for p in port_mappings
TypeError: 'NoneType' object is not iterable

@drewkerrigan drewkerrigan requested review from nlsun and jdef September 13, 2017 16:48
utils.py Outdated
return portMappings

portMappings = app.get('container', {})\
.get('portMappings')
.get('portMappings', [])
Copy link
Contributor

Choose a reason for hiding this comment

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

we should remove the default and then have a specific check to return [] if None. This is because app is may have None as the value.

We partially mitigated this elsewhere with the "cleanup_json" function, but there is one other place in the code that gets json (marathon.list()) which we don't sanitize

Copy link
Contributor

Choose a reason for hiding this comment

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

we can either try to sanitize all entrypoints of json (like marathon.list()) or have this function also handle a None aka json null

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please check #496 before we move forward with this PR

* Remove None from util.py helpers

* Fix ip cache usage, add cleanup_json
@drewkerrigan
Copy link
Contributor Author

@nlsun @jdef Ready for review.

@drewkerrigan
Copy link
Contributor Author

I've manually tested these fixes on DC/OS 1.9 and 1.10-beta2, merging and then releasing a new version.

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

Successfully merging this pull request may close these issues.

2 participants