Skip to content
This repository was archived by the owner on Jun 3, 2021. It is now read-only.

Route53 updater#945

Merged
hugoesthere merged 28 commits into
hh-worker-portsfrom
hh-worker-ports-remote
Jan 9, 2018
Merged

Route53 updater#945
hugoesthere merged 28 commits into
hh-worker-portsfrom
hh-worker-ports-remote

Conversation

@hugoesthere
Copy link
Copy Markdown
Contributor

@hugoesthere hugoesthere commented Jan 4, 2018

Resolves: #869
See: Originate/space-tweet#43 for an example

Tested and works:
screen shot 2018-01-04 at 2 15 34 pm

@hugoesthere hugoesthere requested a review from alexdavid January 4, 2018 22:17
Copy link
Copy Markdown
Contributor

@charlierudolph charlierudolph left a comment

Choose a reason for hiding this comment

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

Looks great to me. I added in #946 so we can add some documentation for how to use this. Please wait for a review from @alexdavid

Comment thread resources/update-route53/main.go Outdated

func main() {
if len(os.Args) < 3 {
panic(errors.New("Not enough arguments. Arguments must be non-empty strings passed into 'route53-updater <service-role> <internal-hosted-zone-name>'"))
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Lets use log.Fataln instead of panic as we don't need to have the stack trace printed

Comment thread resources/update-route53/main.go Outdated
)

func main() {
if len(os.Args) < 3 {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

How about checking if len(os.Args) != 3?

Comment thread terraform/aws/ecs-cluster/iam.tf Outdated
"ecs:StartTask",
"autoscaling:*"
"autoscaling:*",
"route53:ChangeResourceRecordSets",
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think we should restrict this to only allow changes on the internal hosted zone

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

How would we restrict it to only the internal hosted zone in aim roles?

Comment thread resources/update-route53/main.go Outdated
if serviceRole == "" || internalHostedZoneName == "" {
log.Fatalln(errors.New("Service role or internal hosted zone name missing. Both arguments must be non-empty strings passed into 'route53-updater <service-role> <internal-hosted-zone-name>'"))
}
internalIP, err := exec.Command("curl", "-fsSL", "http://169.254.169.254/latest/meta-data/local-ipv4").Output()
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

does it make sense to run this as an external command or just make an http request here?

@hugoesthere hugoesthere merged commit 4393392 into hh-worker-ports Jan 9, 2018
@hugoesthere hugoesthere deleted the hh-worker-ports-remote branch January 9, 2018 19:46
hugoesthere pushed a commit that referenced this pull request Jan 9, 2018
* expose worker ports in local development

* address comments

* Route53 updater (#945)

* address comments

* test

* update

* update

* update

* update

* update

* update

* update

* update

* update

* update

* update

* update

* update

* update

* update

* address comments

* service endpoints

* http call

* restrict policy to internal zones

* move policy

* update

* update

* change resource

* address comment
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.

3 participants