Preparing for workload attributes support #277
Conversation
Signed-off-by: Ziv Nevo <[email protected]>
Signed-off-by: Ziv Nevo <[email protected]>
Signed-off-by: Ziv Nevo <[email protected]>
Signed-off-by: Ziv Nevo <[email protected]>
Signed-off-by: Ziv Nevo <[email protected]>
Signed-off-by: Ziv Nevo <[email protected]>
Signed-off-by: Ziv Nevo <[email protected]>
zivnevo
changed the title
| srcLabels := cp.platform.GetLabelsFromIP(req.IP) | ||
| if src, ok := srcLabels["app"]; ok { // TODO: Add support for labels other than just the "app" key. | ||
| cp.logger.Infof("Received egress authorization srcLabels[app]: %v.", srcLabels["app"]) | ||
| connReq.SrcService = src | ||
| connReq.SrcWorkloadAttrs = policytypes.WorkloadAttrs{policyengine.ServiceNameLabel: src} |
There was a problem hiding this comment.
q: does it make sense to have member structs for ConnectionRequest.Source and ConnectionRequest.Destination instead of prefixing value names (e.g., ConnectionRequest.SrcWorkloadAttrs vs ConnectionRequest.Source.Attributes)?
We can encapsulate some logic if it is a proper type?
There was a problem hiding this comment.
SrcWorkloadAttrs is now the only information on the source, as SrcPeer was removed. If needed, we can encapsulate more logic into policytypes.WorkloadAttrs
| @@ -175,14 +175,11 @@ func (cp *Instance) CreateExport(export *cpstore.Export) error { | |||
| if eSpec.ExternalService != "" && !netutils.IsIP(eSpec.ExternalService) && !netutils.IsDNS(eSpec.ExternalService) { | |||
| return fmt.Errorf("the external service %s is not a hostname or an IP address", eSpec.ExternalService) | |||
| } | |||
| resp, err := cp.policyDecider.AddExport(&api.Export{Name: export.Name, Spec: export.ExportSpec}) | |||
| // TODO: check policyDecider's answer | |||
There was a problem hiding this comment.
nit: can you assign the TODO to yourself in the comment (if that's the right assignment)?
There was a problem hiding this comment.
We have to decide if we want separate export policies or rather derive export behavior from access policies
pkg/controlplane/instance.go
Outdated
| @@ -399,7 +393,7 @@ func (cp *Instance) UpdateBinding(binding *cpstore.Binding) error { | |||
| if err != nil { | |||
| return err | |||
| } | |||
| if action != event.Allow { | |||
| if action != policytypes.PolicyActionAllow { | |||
There was a problem hiding this comment.
Need to consider if we want policies to allow/deny binding as well when moving to CRD based management (the current proposal does NOT have a binding object but instead lists Peers on the Import CRD).
| requestAttr := event.ConnectionRequestAttr{SrcService: svcName, Direction: event.Incoming} | ||
| connReqResp, err := ph.AuthorizeAndRouteConnection(&requestAttr) | ||
| require.Equal(t, event.Allow, connReqResp.Action) | ||
| srcAttrs := policytypes.WorkloadAttrs{policyengine.ServiceNameLabel: svcName} |
There was a problem hiding this comment.
how/where are these retrieved in non-test code? We need to implement some way to get attributes beyond just labels being used now.
Also (and unrelated to this specific location/file), while it may be ok to "send" all properties as map[string]string on the wire, I wonder if we want to make them explicit by defining the individual attributes on some enclosing struct passed between control plane and the policy engine (ie., explicitly have Image, Labels etc. when collected before it is exchanged on the wire with the peer)?
It may help in reasoning in the code, about what is needed and being examined for a policy decision, easier.
There was a problem hiding this comment.
Yes, the current PR is only preparing for the real gathering and transfer of attributes
| ) | ||
|
|
||
| type ConnectionRequest struct { | ||
| SrcPeer string |
There was a problem hiding this comment.
See earlier discussion regarding use of explicit Source and Destination structs to store the relevant attributes and making the attributes explicit in the communication between control plane and policy engine (instead of map[string]string as used for current state where attributes == Labels.
Signed-off-by: Ziv Nevo <[email protected]>
Signed-off-by: Ziv Nevo <[email protected]>
Signed-off-by: Ziv Nevo <[email protected]>
In this PR:
pkg/controlplane/eventmanager/events.gotopolicyengine. Most of them are now inpkg/policyengine/policytypes/connection_request.goAddExport()- will one day return a list of peers to which export is allowed (currently always returns all peers, and the returned value is currently being ignored by the controlplane.References #17