Skip to content

Simplify aws policy by adding cortex prefix to queue name #1941

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Mar 11, 2021
Merged

Simplify aws policy by adding cortex prefix to queue name #1941

merged 3 commits into from
Mar 11, 2021

Conversation

vishalbollu
Copy link
Contributor

@vishalbollu vishalbollu commented Mar 5, 2021 •
edited
Loading

checklist:

  • run make test and make lint
  • test manually (i.e. build/push all images, restart operator, and re-deploy APIs)

miguelvr
miguelvr reviewed Mar 9, 2021
View reviewed changes
docs/clusters/aws/auth.md
@@ -77,7 +77,7 @@ _NOTE: The policy created during `cortex cluster up` will automatically be delet
{
"Effect": "Allow",
"Action": "sqs:*",
"Resource": "arn:aws:sqs:{{ .Region }}:{{ .AccountID }}:{{ .SQSPrefix }}*"
"Resource": "arn:aws:sqs:{{ .Region }}:{{ .AccountID }}:cortex-*"
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: convert to const

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What do you mean here?

Copy link
Collaborator

Choose a reason for hiding this comment

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

The cortex- prefix

miguelvr
miguelvr reviewed Mar 9, 2021
View reviewed changes
miguelvr
miguelvr reviewed Mar 9, 2021
View reviewed changes
pkg/types/clusterconfig/aws_policy.go
@@ -53,7 +53,7 @@ var _cortexPolicy = `
{
"Effect": "Allow",
"Action": "sqs:*",
"Resource": "arn:aws:sqs:{{ .Region }}:{{ .AccountID }}:{{ .SQSPrefix }}*"
"Resource": "arn:aws:sqs:{{ .Region }}:{{ .AccountID }}:cortex-*"
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: convert to const

miguelvr
miguelvr reviewed Mar 9, 2021
View reviewed changes
pkg/types/clusterconfig/cluster_config_aws.go
// 10 was chosen to make sure that other identifiers can be added to the full queue name before reaching the 80 char SQS name limit
return hash.String(clusterName)[:10] + "-"
// 8 was chosen to make sure that other identifiers can be added to the full queue name before reaching the 80 char SQS name limit
return "cortex-" + hash.String(clusterName)[:8] + "-"
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: convert to const

@RobertLucian RobertLucian changed the title Simplify aws policy by add cortex prefix to queue name Simplify aws policy by adding cortex prefix to queue name Mar 9, 2021
@vishalbollu vishalbollu merged commit df2c91d into master Mar 11, 2021
@vishalbollu vishalbollu deleted the remove-sqs-prefix-from-policy branch March 11, 2021 18:56
@vishalbollu vishalbollu added this to the 0.31 milestone Mar 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@miguelvr miguelvr miguelvr left review comments

@deliahu deliahu deliahu approved these changes

@RobertLucian RobertLucian RobertLucian approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v0.31
Development

Successfully merging this pull request may close these issues.
4 participants
@vishalbollu @deliahu @miguelvr @RobertLucian