Skip to content

fix: removed unnecessary conditional in private access security group#915

Merged
dpiddockcmp merged 1 commit into
terraform-aws-modules:masterfrom
craftech-io:fix_private_access_security_group
Jun 10, 2020
Merged

fix: removed unnecessary conditional in private access security group#915
dpiddockcmp merged 1 commit into
terraform-aws-modules:masterfrom
craftech-io:fix_private_access_security_group

Conversation

@arielvinas

@arielvinas arielvinas commented Jun 9, 2020

Copy link
Copy Markdown
Contributor

PR o'clock

Description

Issue: #914

This fix corrects an incorrect behavior, in which the module requires the user to have enable manage_aws_auth variable for the creation of a security_group with the ingress rules necessary to access the private endpoint of the EKS cluster.

Checklist

@dpiddockcmp dpiddockcmp changed the title Fix: removed unnecessary conditional in private access security group fix: removed unnecessary conditional in private access security group Jun 10, 2020

@dpiddockcmp dpiddockcmp left a comment

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.

Thank you for your PR

@dpiddockcmp dpiddockcmp merged commit 348f441 into terraform-aws-modules:master Jun 10, 2020
@anthonydahanne

anthonydahanne commented Aug 18, 2020

Copy link
Copy Markdown
Contributor

hello
I'm using 12.2.0 and I can't find a way to disable this security group creation.
Back in 12.1.0, I did not need it and was doing fine without it.
But because of this change, I can't find a way to prevent the creation of this security group rule.

Even worse, because I have a limited access to the underlying AWS account, I'm stuck with this error:

module.eks.aws_security_group_rule.cluster_private_access[0]: Creating...

Error: Error authorizing security group rule type ingress: UnauthorizedOperation: You are not authorized to perform this operation. Encoded authorization failure message: TXgpzCB6noNIFS4     [.....]  WVUtmemnuvt2UFhHAU
        status code: 403, request id: 5f92d5   [.....]    566

Can you please revert this breaking change?
Thank you

@barryib

barryib commented Aug 18, 2020

Copy link
Copy Markdown
Member

@anthonydahanne This PR "corrects" an incorrect behavior. The SG creation doesn't have any relation with the managed_aws_auth. The SG you're referring to is created only for private EKS endpoint.

But maybe the correct one should use var.cluster_create_security_group instead of var.manage_aws_auth ? @dpiddockcmp do you confirm ?

@anthonydahanne

Copy link
Copy Markdown
Contributor

thanks @barryib for your comment!

Indeed, it seems more natural to use the variable named var.cluster_create_security_group instead of var.manage_aws_auth to create.... a security group rule!

It's not because users have a private EKS endpoint that they necessarily need the creation of security groups.

I don't think it's uncommon to have users, like me, interacting with an AWS account that has most VPC resources already in place, without any rights to create additional ones.

Shall I go ahead and create an issue and corresponding PR ?

@barryib

barryib commented Aug 18, 2020

Copy link
Copy Markdown
Member

Shall I go ahead and create an issue and corresponding PR ?

Yes please.

@anthonydahanne

Copy link
Copy Markdown
Contributor

there it is @barryib :

Issue: #980
PR: #981

thanks for your suggestion!

barryib pushed a commit to Polyconseil/terraform-aws-eks that referenced this pull request Oct 25, 2020
@github-actions

Copy link
Copy Markdown

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions Bot locked as resolved and limited conversation to collaborators Nov 17, 2022
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.

4 participants