-
Notifications
You must be signed in to change notification settings - Fork 25
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
Add support for NetworkACLs for LB on VPC networks #69
Conversation
need to upload the image to apache docker repo - before it's merged |
// create ACL rule | ||
acl := lb.NetworkACL.NewCreateNetworkACLParams(protocol.CSProtocol()) | ||
acl.SetAclid(network.Aclid) | ||
acl.SetAction("Allow") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you make these values parametrized? (action, cidr, trafficType) - it looks like you currently want them for adding the default ACL allow but it can be reused later if parameters are used
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ideally should not be required. Because, these rules correspond to the LB service created on Kubernetes side. So these would ideally be fixed to action: "allow" and traffictype: "Ingress"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
code lgtm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code LGTM, thanks @Pearl1594
// create ACL rule | ||
acl := lb.NetworkACL.NewCreateNetworkACLParams(protocol.CSProtocol()) | ||
acl.SetAclid(network.Aclid) | ||
acl.SetAction("Allow") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not working on a vmware environment
It works fine on a Cloudstack +kvm environment
Let me know If i should create a separate issue(Cloudstack+vmware) to track it
@kiranchavala I don't think the issue is wrt to this PR, but in general there's an issue with vmware. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, Tested the cks deployment on a vpc network custom acl.
The rules were successfully added/removed for kubernetes deployments
LGTM - thanks @Pearl1594 |
This adds support to create and delete Network ACLs as done for Isolated networks (where Firewall rules are added)
On cluster creation:
On deploying nginx
kubectl apply -f <nginx.yaml>
On deleting nginx
kubectl delete -f <nginx.yaml>