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.

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

[SecurityGroups] Cannot create security group when referencing itself inside a security group rule #2061

Closed
adriananeci opened this issue Apr 23, 2024 · 9 comments
Closed

[SecurityGroups] Cannot create security group when referencing itself inside a security group rule #2061

adriananeci opened this issue Apr 23, 2024 · 9 comments
Labels
service/ec2 Indicates issues or PRs that are related to ec2-controller.

Comments

@adriananeci
Copy link

Describe the bug
Security group creation is failing when referencing itself inside a security group rule

- lastTransitionTime: "2024-04-23T15:27:52Z"
  message: Reference resolution failed
  reason: the referenced resource is not synced yet. resource:SecurityGroup, namespace:sbx-clusters, name:ci-sbx-or2-sg-k8s-all
  status: Unknown
  type: ACK.ReferencesResolved

Steps to reproduce
Try to create a security group referencing itself inside a rule using something like

apiVersion: ec2.services.k8s.aws/v1alpha1
kind: SecurityGroup
metadata:
  annotations:
    services.k8s.aws/region: us-west-2
  name: ci-sbx-or2-sg-k8s-all
spec:
  description: Applied to all instances for outbound access
  egressRules:
  - fromPort: 1
    ipProtocol: tcp
    toPort: 65535
    userIDGroupPairs:
    - description: Allow all nodes to communicate
      groupRef:
        from:
          name: ci-sbx-or2-sg-k8s-all
      userID: "1234"
      vpcRef:
        from:
          name: ci-sbx-or2-k8s
  ingressRules:
  - fromPort: 1
    ipProtocol: tcp
    toPort: 65535
    userIDGroupPairs:
    - description: Allow all nodes to communicate
      groupRef:
        from:
          name: ci-sbx-or2-sg-k8s-all
      userID: "1234"
      vpcRef:
        from:
          name: ci-sbx-or2-k8s
  name: ci-sbx-or2-sg-k8s-all
  tags:
  - key: Name
    value: ci-sbx-or2-k8s-k8s-all
  - key: AckName
    value: ci-sbx-or2-sg-k8s-all

Expected outcome

Security group should be able to reference itself

Environment

  • Kubernetes version 1.28
  • Using EKS (yes/no), if so version? 1.28
  • AWS service targeted (S3, RDS, etc.) EC2
@a-hilaly a-hilaly added the service/ec2 Indicates issues or PRs that are related to ec2-controller. label Apr 25, 2024
@a-hilaly
Copy link
Member

@adriananeci ReferenceResolver expects the referenced resource to be fully synced before being able to use it's values, this creates a sort of circular dependency. There are two options here:

  • Create the securitygroup first, wait for it to be synced, then add egress/ingress rules.
  • For a more gitops friendly experience, we might to start defaulting the GroupID when none of the GroupID/GroupName are mentioned.

@a-hilaly a-hilaly mentioned this issue Jun 3, 2024
Merged
@a-hilaly
Copy link
Member

a-hilaly commented Jun 3, 2024

@adriananeci v1.2.9 will include a fix. Instead of using a self reference, you can just leave the field empty, the controller will automatically infer the GroupID/VPCID from the SecurityGroup specification.

ack-prow bot pushed a commit to aws-controllers-k8s/ec2-controller that referenced this issue Jun 3, 2024
@a-hilaly
Set correct userIDGroupPairs defaults for SecurityGroups CRs. (#194)
68c5e39 
Closes aws-controllers-k8s/community#2068,
aws-controllers-k8s/community#2061, and
aws-controllers-k8s/community#2058

The EC2 API for setting ingress/egress rules has many special restrictions,
making its behavior hard to predict. For example, `GroupName` should only be
used with default VPCs. When using non default VPCs users should use `GroupID`
instead

To address this problem, we are introducing a defaulting mechanism to help the
controller infer and use the correct `GroupID` when a user doesnt provide one.

You might wonder why all the trouble, and why not just use ACK resource references?
Well.. this is necessary because ACK resource references cannot do self
references, making fully declarative egress/ingress rule definition impossible in some
cases.

Changes:
- Mark `UserIDGroupPairs.GroupName` as non-required (at the CRD level)
- Default `UserIDGroupPairs.GroupID` to the parent security group ID
- Default `UserIDGroupPairs.VPCID` to the VPC of the parent security group
- Add more e2e tests for `UserIDGroupPairs`

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
@a-hilaly a-hilaly closed this as completed Jun 3, 2024
@adriananeci
Copy link
Author

/reopen

@ack-prow ack-prow bot reopened this Jul 11, 2024
@ack-prow ACK Prow
Copy link

ack-prow bot commented Jul 11, 2024

@adriananeci: Reopened this issue.

In response to this:

/reopen

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@adriananeci
Copy link
Author

adriananeci commented Jul 11, 2024 •
edited
Loading

@a-hilaly The problem still persists. I've tried omitting or leaving empty different fields from inside the userIDGroupPairs but with no luck. For example for a spec like:

apiVersion: ec2.services.k8s.aws/v1alpha1
kind: SecurityGroup
metadata:
  annotations:
    services.k8s.aws/region: us-west-2
  name: aneci-sbx-va6-sg-k8s-all
spec:
  description: Applied to all instances for outbound access
  egressRules:
  - fromPort: 1
    ipProtocol: tcp
    toPort: 65535
    userIDGroupPairs:
    - description: Allow all nodes to communicate
      vpcRef:
        from:
          name: aneci-sbx-va6-k8s
  ingressRules:
  - fromPort: 1
    ipProtocol: tcp
    toPort: 65535
    userIDGroupPairs:
    - description: Allow all nodes to communicate
      vpcRef:
        from:
          name: aneci-sbx-va6-k8s
  name: aneci-sbx-va6-sg-k8s-all
  tags:
  - key: Name
    value: aneci-sbx-va6-k8s-k8s-all
  - key: AckName
    value: aneci-sbx-va6-sg-k8s-all

the object status is getting into

Status:
  Conditions:
    Last Transition Time:  2024-07-11T13:28:11Z
    Message:               Reference resolution failed
    Reason:                resource reference wrapper or ID required: VPCID,VPCRef
    Status:                Unknown
    Type:                  ACK.ReferencesResolved

Do you have a working security group example spec that is referencing itself? Maybe I'm missing something.

I've been running the tests using ec2-controller v1.2.13

@jantzenallphin
Copy link

I've looked into this in the last week and found the below to be a working example.

apiVersion: ec2.services.k8s.aws/v1alpha1
kind: SecurityGroup
metadata:
  annotations:
    services.k8s.aws/region: us-east-1
  finalizers:
  - finalizers.ec2.services.k8s.aws/SecurityGroup
  name: jallphin00-sbx-va6-sg-k8s-control-plane
  namespace: sbx-clusters
spec:
  description: k8s-control-plane security group rules for jallphin00-sbx-va6
  egressRules:
  - fromPort: 1025
    ipProtocol: tcp
    toPort: 65535
    userIDGroupPairs:
    - description: Allow traffic from control plane to workers
      userID: "<account id>"
  - fromPort: 443
    ipProtocol: tcp
    toPort: 443
    userIDGroupPairs:
    - description: Allow HTTPS traffic from control plane to workers
      userID: "<account id>"
  name: jallphin00-sbx-va6-sg-k8s-control-plane
  tags:
  - key: Name
    value: jallphin00-sbx-va6-sg-k8s-control-plane
  - key: AckName
    value: jallphin00-sbx-va6-sg-k8s-control-plane
  vpcID: <vpc id>

@adriananeci I believe the issue was the vcpRef you still had in your last posted example. You can leave this blank due to what @a-hilaly mentioned here.

After applying above manifest, I've checked the created securitygroup in the console does indeed show reference to itself in these egress rules.

/close

@ack-prow ACK Prow
Copy link

ack-prow bot commented Aug 2, 2024

@jantzenallphin: You can't close an active issue/PR unless you authored it or you are a collaborator.

In response to this:

I've looked into this in the last week and found the below to be a working example.

apiVersion: ec2.services.k8s.aws/v1alpha1
kind: SecurityGroup
metadata:
 annotations:
   services.k8s.aws/region: us-east-1
 finalizers:
 - finalizers.ec2.services.k8s.aws/SecurityGroup
 name: jallphin00-sbx-va6-sg-k8s-control-plane
 namespace: sbx-clusters
spec:
 description: k8s-control-plane security group rules for jallphin00-sbx-va6
 egressRules:
 - fromPort: 1025
   ipProtocol: tcp
   toPort: 65535
   userIDGroupPairs:
   - description: Allow traffic from control plane to workers
     userID: "<account id>"
 - fromPort: 443
   ipProtocol: tcp
   toPort: 443
   userIDGroupPairs:
   - description: Allow HTTPS traffic from control plane to workers
     userID: "<account id>"
 name: jallphin00-sbx-va6-sg-k8s-control-plane
 tags:
 - key: Name
   value: jallphin00-sbx-va6-sg-k8s-control-plane
 - key: AckName
   value: jallphin00-sbx-va6-sg-k8s-control-plane
 vpcID: <vpc id>

@adriananeci I believe the issue was the vcpRef you still had in your last posted example. You can leave this blank due to what @a-hilaly mentioned here.

After applying above manifest, I've checked the created securitygroup in the console does indeed show reference to itself in these egress rules.

/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@a-hilaly
Copy link
Member

a-hilaly commented Aug 2, 2024

Thank you foklks!
/close

@ack-prow ack-prow bot closed this as completed Aug 2, 2024
@ack-prow ACK Prow
Copy link

ack-prow bot commented Aug 2, 2024

@a-hilaly: Closing this issue.

In response to this:

Thank you foklks!
/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

nnbu pushed a commit to nnbu/ack-ec2-controller that referenced this issue Sep 18, 2024
@a-hilaly
Set correct userIDGroupPairs defaults for SecurityGroups CRs. (aw…
e53c674 
…s-controllers-k8s#194)

Closes aws-controllers-k8s/community#2068,
aws-controllers-k8s/community#2061, and
aws-controllers-k8s/community#2058

The EC2 API for setting ingress/egress rules has many special restrictions,
making its behavior hard to predict. For example, `GroupName` should only be
used with default VPCs. When using non default VPCs users should use `GroupID`
instead

To address this problem, we are introducing a defaulting mechanism to help the
controller infer and use the correct `GroupID` when a user doesnt provide one.

You might wonder why all the trouble, and why not just use ACK resource references?
Well.. this is necessary because ACK resource references cannot do self
references, making fully declarative egress/ingress rule definition impossible in some
cases.

Changes:
- Mark `UserIDGroupPairs.GroupName` as non-required (at the CRD level)
- Default `UserIDGroupPairs.GroupID` to the parent security group ID
- Default `UserIDGroupPairs.VPCID` to the VPC of the parent security group
- Add more e2e tests for `UserIDGroupPairs`

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
service/ec2 Indicates issues or PRs that are related to ec2-controller.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@a-hilaly @adriananeci @jantzenallphin