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

Add platform-specific settings to API model #636

Merged
merged 1 commit into from
Feb 1, 2020
Merged

Conversation

tjkirch
Copy link
Contributor

@tjkirch tjkirch commented Jan 10, 2020

Initially, this means setting "region" for the AWS platform.

The first use case is 'region' so we can regionalize container addresses; usage
will be controlled by variant-specific (and therefore platform-specific)
metadata, so a platform-specific setting path doesn't hurt. In the near
future, we will likely want to use region in other places, like telemetry,
where we don't want applications to have to understand platform; in this case,
we can abstract usage through a sundog generator that translates platform
specifics to application-specific settings.

Implementation is mostly in moondog. Previously, we only read user data, so it
needed some restructuring to handle other data in IMDS. Now, each platform
implements PlatformDataProvider. Its sole method can read user data or other
metadata however it likes, and just has to return a list of Settings-like
objects that it wants to be sent to the API.


Testing done:

The system is healthy, pods launch OK, moondog had no errors, and the new setting shows up:

[ec2-user@ip-192-168-33-74 ~]$ apiclient -u /settings
{
...
  "aws": {
    "region": "us-east-1"
  }
}

workspaces/api/moondog/README.md Show resolved Hide resolved
workspaces/api/moondog/README.md Show resolved Hide resolved
workspaces/api/moondog/src/main.rs Show resolved Hide resolved
Copy link
Contributor

@zmrow zmrow left a comment

Choose a reason for hiding this comment

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

🏔

@tjkirch tjkirch requested a review from bcressey January 21, 2020 22:31
workspaces/api/moondog/src/main.rs Outdated Show resolved Hide resolved
workspaces/api/moondog/src/main.rs Outdated Show resolved Hide resolved
workspaces/api/moondog/src/main.rs Outdated Show resolved Hide resolved
@tjkirch
Copy link
Contributor Author

tjkirch commented Jan 23, 2020

This push addresses Ben's concerns by:

  • making the identity doc optional like user data, because the user can disable the IMDS endpoint now with IMDSv2
  • Making the "instance identity document" wording more consistent in messaging, and use shorter forms like "identity doc" in variables / places where space is at a premium

I made a new AMI to confirm things still plumb through.

@tjkirch tjkirch requested a review from zmrow January 28, 2020 17:28
Copy link
Contributor

@zmrow zmrow left a comment

Choose a reason for hiding this comment

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

🗻

Initially, this means setting "region" for the AWS platform.

The first use case is 'region' so we can regionalize container addresses; usage
will be controlled by variant-specific (and therefore platform-specific)
metadata, so a platform-specific setting path doesn't hurt.  In the near
future, we will likely want to use region in other places, like telemetry,
where we don't want applications to have to understand platform; in this case,
we can abstract usage through a sundog generator that translates platform
specifics to application-specific settings.

Implementation is mostly in moondog.  Previously, we only read user data, so it
needed some restructuring to handle other data in IMDS.  Now, each platform
implements PlatformDataProvider.  Its sole method can read user data or other
metadata however it likes, and just has to return a list of Settings-like
objects that it wants to be sent to the API.
@tjkirch
Copy link
Contributor Author

tjkirch commented Jan 28, 2020

This push fixes the typo caught by @webern.

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.

4 participants