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

handling selectors with matchexpressions (fixed) #377

Draft
wants to merge 7 commits into
base: new_exposure_analysis_first_branch
Choose a base branch
from

Conversation

shireenf-ibm
Copy link
Contributor

#236
task:

  • support selectors with matchExpression (all operators)

@shireenf-ibm shireenf-ibm requested a review from adisos June 23, 2024 18:09
@shireenf-ibm shireenf-ibm marked this pull request as draft June 23, 2024 18:10
@shireenf-ibm shireenf-ibm changed the title New handling selectors with matchexpressions handling selectors with matchexpressions (fixed) Jun 23, 2024
Copy link
Collaborator

@adisos adisos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

few initial comments

pkg/netpol/connlist/conns_formatter.go Outdated Show resolved Hide resolved
pkg/netpol/connlist/conns_formatter.go Outdated Show resolved Hide resolved
pkg/netpol/connlist/conns_formatter.go Outdated Show resolved Hide resolved
pkg/netpol/connlist/conns_formatter.go Outdated Show resolved Hide resolved
pkg/netpol/connlist/conns_formatter.go Outdated Show resolved Hide resolved
@adisos
Copy link
Collaborator

adisos commented Jun 24, 2024

general comment: the expression selectors could be represented as LabelSelectorRequirement from pkg go\pkg\mod\k8s.io\[email protected]\pkg\apis\meta\v1\types.go, instead of converting to map[string]string and then re-converting to a string?
why do we need our own conversion? could just add a separate field for such selectors of this type instead?

@shireenf-ibm
Copy link
Contributor Author

general comment: the expression selectors could be represented as LabelSelectorRequirement from pkg go\pkg\mod\k8s.io\[email protected]\pkg\apis\meta\v1\types.go, instead of converting to map[string]string and then re-converting to a string? why do we need our own conversion? could just add a separate field for such selectors of this type instead?

I thought about this either but found that it will cost multiple changes and will not differ that much for the "special" cases
1* we can add fields of "LabelSelectorRequirement" to the k8s.Pod and eval.RepresentativePeer interfaces and consider them each time needed; but still the ExposedPeer will contain the representative labels as map[string]string (so preferred to be consistent from beginning) 2* operator Incase; I preferred to convert the expression into labels of <key:val> for each value (and so a new representative peer is created for each) In case of the other operators it was only to convert the to a single <key>:<val> where the <val> is "special" and needs to be "compared" specifically , (same comparisons would be done in case we use theLabelSelectorRequirement` )

@shireenf-ibm
Copy link
Contributor Author

some comments regarding the last commit:
current code changes:

  • since the potential namespaces and pods a peer may be exposed to can contain either labels map or list of requirements
    I used the LabelSelector struct to represent them.(it includes both map[string]string and []LabelSelectorRequirement)

  • however , in order to print the potential labels and requirements string I prefered to use the labels string func and LabelSelectorRequirement.String() func rather than the LabelSelector.String() ; in order to get a neater output.

  • even though LabelSelector struct includes both labels and requirements fields; in k8s.pod and k8s.namespace kept two different fields for each (since real pods and namespaces has only labelsMap)

a suggestion: (not implemented here)

  • since k8s.namespace contains both requirements and labels; we have the option to remove the RepresentativePeer struct and use PodPeer struct to represent representative peer, by differentiating the kind value of the peer; (if this is desired, i prefer to do in a different PR)

@shireenf-ibm shireenf-ibm requested a review from adisos June 27, 2024 12:24
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you also duplicate these tests for scenarios with actual pods also matched by these selectors?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@shireenf-ibm shireenf-ibm requested a review from adisos June 29, 2024 21:37
if isRepresentativePod(peer) {
// representative peer's namespace labels may be inferred from a rule with special matchExpression requirements
// and also contains the representative ns name label which is not relevant for comparison
peerMatchesNamespaceSelector, err = SelectorMatchesRepresentativePeerLabels(selector, peerNamespace.Labels,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can't we just keep a reference to the selector(s) from which this representative peer was created, and consider a match only if this is the relevant selecotr, instead of implementing containment of selectors comparison?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we still need this containment, some selectors may intersect / be equivalent on one way to other selectors
i.e. a representative peer (built from one selector) may match two or more selectors actually and then the connection contains the ports of all the selectors.
an example : test_exposure_with_different_rules_6 (there are more examples like this too)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants