-
Notifications
You must be signed in to change notification settings - Fork 43
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 uuid service support #117
base: develop
Are you sure you want to change the base?
Conversation
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 start, please check my comments, I think there are lot of things we can change here.
client util.PangoClient | ||
} | ||
|
||
func NewService(client util.PangoClient) *Service { |
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.
There is lack of validation for constructor.
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 can't fail, so it doesn't need to return an error..? What validation did you have in mind?
} | ||
} | ||
|
||
func (e *Entry) Field(value string) (any, error) { |
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 think the better approach is to actually go with reflect in this case, this whole logic we can put in few lines of code to generate what we need.
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 very well could use reflect, but as I brought up in our meeting this function should also be able to deal with other situations, such as indexing into a list of structs (neither address objects nor security rules has this situation, so there is no example of this yet).
My recommendation is to pursue reflect, but after all the other work is done.
} | ||
} | ||
|
||
func (e *Entry) Field(value string) (any, error) { |
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 clarify I would return 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.
any
is a synonym for interface{}
.
return nil, fmt.Errorf("unknown field: %s", value) | ||
} | ||
|
||
func Versioning(vn version.Number) (Specifier, Normalizer, error) { |
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 is commented here.
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.
Yes, this is because security rules should actually have a second versioned object for PAN-OS 10.2.0, but actually adding that support in to this PR felt like it would make the PR too unwieldy. So this comment exists as a reminder that a second object for PAN-OS > 10.2.0 should exist, it's just not in this PR. This is the DisableInspect
param on line 59.
} | ||
|
||
type Entry1Container struct { | ||
Answer []Entry1 `xml:"entry"` |
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 don't follow what this var is responsible for. (Just naming stuff)
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.
Is this comment talking about line 407 or something else?
Misc []generic.Xml `xml:",any"` | ||
} | ||
|
||
func SpecMatches(a, b *Entry) bool { |
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 have strong feeling that this function can be simplify as well.
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.
We discussed a bit in Friday's meeting. I also recommend pursuing this once we are farther along on the generator as a whole.
} | ||
|
||
// profile settings | ||
if a.ProfileSettings == nil && b.ProfileSettings == nil { |
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.
You create empty block, which doesn't make sense since there is nothing even to return here.
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.
We are looking for equality, so there are three situations:
- both are nil: they match
- one is nil while the other isn't: they don't match
- both are non-nil: you have to dive into the struct to verify equality
Thus this empty block is just the first situation, so there is nothing to do other than move on to other fields.
I understand it's weird to see an empty block, but it is doing the right thing here. Is there something that you would like to see here instead?
…p placement logic
…g function that will exist in the panos provider
There are multiple things that differ with this implementation than the address object implementation.
[Within Location]
Location.Xpath()
function takes three params instead of two; the third parameter being a UUID to use for XPATH purposes instead of the name.[Within Entry]
Entry.Field()
shows how an implementation of retrieving a field from a sub-struct could look like.Entry.SpecMatches()
shows how an implementation of sub-struct matching could look like.Entry.DisableServerResponseInspection
is a flat param with other params but in the XPATH is actually nested within anOptions
section in the XML.LogEnd
param requires a custom UnmarshalXML, and the generator needs to be able to do this.Entry.DisableInspect
param should result in a second versioned object when this output from the generator.Entry.Targets
is a unique type that any rule in the Policy section will have, so we'll need the generator to be able to handle this param type.[Within service.go]
Service.ReadGroup()
/Service.ConfigureGroup()
/Service.DeleteGroup()
.ConfigureGroup()
can be improved to minimize the number of moves needed to end at the desired configuration. Note that only rules in the group given should be moved so as not to cause configuration drift of other group resources.ConfigureGroup()
- it does two multi-config commands currently, at most: one to update the rule specs themselves and one to do the rule movements. It should be technically possible to combine this into a single multi-config, but it will require careful attention to later parts of the code if thelisting
will be updated in place.I might be missing some other callouts, and this is still missing stuff, such as rule hit count handling, so it's not completely finalized, but it's worth checking out where things stand so far.