-
Notifications
You must be signed in to change notification settings - Fork 260
feat: [NPM] GoalState processor for Daemon pod #1187
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
Conversation
| dp.DeleteIPSet(ipsets.TestKVPodSet.Metadata) | ||
| panicOnError(dp.ApplyDataPlane()) | ||
|
|
||
| panicOnError(dp.AddToLists([]*ipsets.IPSetMetadata{ipsets.TestNestedLabelList.Metadata}, []*ipsets.IPSetMetadata{ipsets.TestKVPodSet.Metadata, ipsets.TestNSSet.Metadata})) |
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 surround this in an if statement (if includeLists or something like other list operations in the file)
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.
It is a validate test case we should use always right ? Any particular reason for this suggestion ?
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.
My machine doesn't support lists, so I introduced includeLists
npm/pkg/dataplane/ipsets/ipset.go
Outdated
| } | ||
|
|
||
| func (set *IPSet) GetSetMetadata() *IPSetMetadata { | ||
| return NewIPSetMetadata(set.unPrefixedName, set.Type) |
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.
why do we want the unprefixed name for this?
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.
Metadata of a ipset should always be created with unprefixed name, or else we will end up using double prefixed names.
| type GenericDataplane interface { | ||
| InitializeDataPlane() error | ||
| ResetDataPlane() error | ||
| GetIPSet(setName string) *ipsets.IPSet |
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.
As discussed offline, I feel this shouldn't be part of the generic dataplane interface. Perhaps the gsp should use the dataplane struct instead of the GenericDataplane interface
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 a function we will need for DPmocks for UTs and also for GSProcessor. So kept it as a higher level interface method. In future, we can this function for other utilities, like debugging or observability.
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.
So is this a no-op in production?
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.
GetIPSet is being used by GSProcessors, DP, to figure out what is existing set goal state and to compute diff. We are also using this in UTs.
Atm, we do not use this function in normal coupled mode.
| return | ||
| } | ||
|
|
||
| err := gsp.processIPSetsApplyEvent(payload[cp.IpsetApply]) |
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.
Check if key exists? We don't want to panic if the event payload doesn't contain the key.
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.
I am a little confused by these process blocks. Does every incoming payload have all keys in the map populated?
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.
For NameSpace resource events:
Add: IPSETAPPLY
Update/Delete: IPSETAPPLY and /(or) IPSETREMOVE
For Pod resource events:
Add: IPSETAPPLY
Update/Delete: IPSETAPPLY and /(or) IPSETREMOVE
For Network Policy resource events:
Add: IPSETAPPLY and PolicyAPPly
Update: IPSETAPPLY and /(or) IPSETREMOVE + PolicyAPPLY
Delete: IPSETREMOVE + PolicyRemove
This is how the keys will match to respective resource events ... @huntergregory FYI
| type GenericDataplane interface { | ||
| InitializeDataPlane() error | ||
| ResetDataPlane() error | ||
| GetIPSet(setName string) *ipsets.IPSet |
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.
So is this a no-op in production?
| outChannel chan *protos.Events | ||
| } | ||
|
|
||
| func NewDPSim(outChannel chan *protos.Events) *DPShim { |
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.
nit: typo
nitishm
left a comment
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.
Good stuff - LGTM
Reason for Change:
Issue Fixed:
Requirements:
Notes: