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

Enforce lowercase service names #97

Merged
merged 3 commits into from
May 31, 2022
Merged

Conversation

pglass
Copy link

@pglass pglass commented May 26, 2022

Changes proposed in this PR:

With the auth method, the service token relies on a service identity, which requires a lowercase name. This updates the binary to:

  • Validate service names are lowercase in the schema.json. Actually, require they match the service identity regex. I also handled this for the gateway service name and update schema validation for this.
  • When no service name is specified, fall back to the lowercase family name.

How I've tested this PR:

Unit and acceptance tests

How I expect reviewers to test this PR:

👀

Checklist:

  • Tests added
  • CHANGELOG entry added

@pglass pglass requested review from a team and cthain and removed request for a team May 26, 2022 15:53
@@ -62,7 +62,8 @@
"properties": {
"name": {
"description": "The name the service will be registered as in Consul. Defaults to the Task family name if empty or null.",
"type": ["string", "null"]
"type": ["string", "null"],
"pattern": "(^$)|(^[a-z0-9]([a-z0-9-_]*[a-z0-9])?$)"
Copy link
Author

Choose a reason for hiding this comment

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

This is empty string (^$) or the service identity regex from Consul.

@@ -357,37 +358,38 @@
},
"lanAddress": {
"description": "LAN address and port for the gateway. If not specified, defaults to the task/node address.",
"type": "object",
"type": ["object", "null"],
Copy link
Contributor

Choose a reason for hiding this comment

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

Question: I'm not clear on why null is defined as a valid type when the fields themselves aren't required. Is this to allow for the possibility to define the config like "lanAddress": null?

Copy link
Author

@pglass pglass May 31, 2022

Choose a reason for hiding this comment

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

Yeah, it's kind of silly, but I setup the config schema this way because it slightly simplifies passing fields from the Terraform module. We don't have to worry in Terraform whether a field is null or a value (string, list, int, etc) for it to pass validation, since there are so many fields we don't specify by default.

It may not make as much sense for some of these fields. And we've stopped using null so much for default values in the Terraform module, so maybe we could rethink this.

@pglass pglass temporarily deployed to dockerhub/hashicorpdev May 31, 2022 15:55 Inactive
@pglass pglass merged commit c9a974b into main May 31, 2022
@pglass pglass deleted the pglass/lowercase-service-names branch May 31, 2022 16:03
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.

2 participants