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

Version 3 fix secgroup case #86

Closed

Conversation

pcn
Copy link

@pcn pcn commented Dec 19, 2011

If I provide the security group "webservice" to the cluster_chef cloud module, and the actual EC2 security group is "Webservice" then knife cluster bootstrap will crash out without a useful message. This patch catches the exception and checks whether case-sensitivity is the issue. If it can find a case-insensitive match, it fixes up the name and allows the bootstrap to continue with a message to the console.

@mrflip
Copy link
Member

mrflip commented Dec 20, 2011

I'd rather fix the 'get' method to look in its array for a case-insensitive match. This class does discovery different than any of the others -- it should: fetch the full list once, populating in directly when we create.

I'm also a little wary of changing amazon's semantics. What is the argument for having the cloud group and the local group having different names?

@pcn
Copy link
Author

pcn commented Dec 20, 2011

I'd rather fix the 'get' method to look in its array for a case-insensitive match. This class does discovery different than any of the others -- it should: fetch the full list once, populating in directly when we create.

This is probably a better and more correct approach. Especially if it's memoized it could save some round-trips.

I'm also a little wary of changing amazon's semantics. What is the argument for having the cloud group and the local group having different names?

It seems that inter-service security groups are case-insensitive. Specifically my issue came from a scenario where I realized that I was installing systems that couldn't talk to my staging database. So I looked at the permissions for my staging database and found this:

1298 pn@PN-mac 13:10 ~/tmp/RDSCli-1.4.007/bin $ ./rds-describe-db-security-groups | grep -A4 rds-misc
SECGROUP  rds-misc      For DBs intended for misc activities
      EC2-SECGROUP  application-apiexample  <ACCNT #>  authorized
      EC2-SECGROUP  cron                    <ACCNT #>  authorized
      EC2-SECGROUP  nagios                  <ACCNT #>  authorized
      EC2-SECGROUP  webservice              <ACCNT #>  authorized

You'll notice that all of these security groups are lowercased. However, that's not how we define them:

1315 pn@PN-mac 13:15 ~/tmp/RDSCli-1.4.007/bin $ ec2-describe-group | grep -i 'webservice' | grep -v -- '-'  | grep GROUP
GROUP   <ACCNT #>    Webservice      Wevservices server

So internally AWS seems to rely on the case-insensitive namespace. Given that they rely on it I figured it was better to roll with it and let users cut-and-paste output from other AWS tools into the cluster chef definitions instead of crashing and making them search for the case-sensitive name.

@pcn
Copy link
Author

pcn commented Dec 20, 2011

If you're OK with the case-insensitivity I'll re-work this to do a single initial fetch of all groups and cache it in the object and then populate the group list based on cached list.

@pcn
Copy link
Author

pcn commented Dec 22, 2011

BTW, there's no hurry on this. I'm going to move on to some other things like looking into the policies on circulating through the AZs when starting up clusters and re-trying when errors are returned in certain AZs (I'm seeing a lot of issues with us-east-1b and us-east-1c saying that a t1.micro can't be allocated which makes testing difficulty).

Let's leave this for now and continue after the new year.

@temujin9
Copy link
Contributor

Any chance you've got an updated patch for this bug?

@pcn
Copy link
Author

pcn commented Feb 23, 2012

Is it acceptable now for groups to be treated as case-insensitive for the purpose of matching?

@pcn
Copy link
Author

pcn commented Mar 21, 2012

Ping?

@temujin9
Copy link
Contributor

I'll be looking into this further, soon. If you happen to have an updated pull request, now would be the time to submit it . . .

@pcn
Copy link
Author

pcn commented Apr 12, 2012

It's been working fine for me, I haven't been able to re-visit this.

@temujin9 temujin9 closed this in 94d7c7b Apr 12, 2012
@temujin9
Copy link
Contributor

Let me know if this fix doesn't work for you; I've tested against our setup, but not extensively.

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.

3 participants