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

Uses the new parsing structure for RBAC parsing #3206

Merged
merged 16 commits into from
Sep 16, 2024

Conversation

jaronoff97
Copy link
Contributor

Description: Uses the new parsing structure for getting parsing rules

Link to tracking Issue(s):

  • Resolves: n/a

Testing: Many unit tests

Documentation: n/a

@jaronoff97 jaronoff97 added the Skip Changelog PRs that do not require a CHANGELOG.md entry label Aug 7, 2024
@iblancasa
Copy link
Contributor

Is there anything I can do to help you with this?

Comment on lines 51 to 60
type Option[T any] struct {
protocol corev1.Protocol
appProtocol *string
targetPort intstr.IntOrString
nodePort int32
name string
port int32
portParser PortParser[T]
rbacGen RBACRuleGenerator[T]
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note: I need the generics for the parsing stuff, but it really crowds the type signatures. (see the receivers/helpers.go file) I could remove the functions from the options struct, and put it in the constructor, but that feels counter intuitive to the options struct.

@@ -75,22 +83,32 @@ func (o *Option) GetServicePort() *corev1.ServicePort {
}
}

type PortBuilderOption func(*Option)
func WithRBACRuleGenerator[T any](r RBACRuleGenerator[T]) PortBuilderOption[T] {
Copy link
Contributor

Choose a reason for hiding this comment

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

TBH I don't feel confortable with everything returing "PortBuilder". For some components we don't have to build anything port related (some processors) but we need to get RBAC. I was not sure about this in the other PR but I though you were thinking about something different and didn't raise my concerns.

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 i should just rename this to not be "PortBuilderOption" but rather "ParserOption" which is more accurate.

type PortRetriever interface {
GetPortNum() (int32, error)
GetPortNumOrDefault(logr.Logger, int32) int32
}

type Option struct {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

moved this to builder.go

Comment on lines +44 to +46
if err := mapstructure.Decode(config, &parsed); err != nil {
return nil, err
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

By using generics here, we can let mapstructure decode take care of all the marshalling for us into the exact right type – no more map assertions!

Copy link
Contributor

@iblancasa iblancasa left a comment

Choose a reason for hiding this comment

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

Great work! Thanks for taking care of this. When do we plan to start using the new infra and remove the old one?

@jaronoff97 jaronoff97 marked this pull request as ready for review August 27, 2024 17:20
@jaronoff97 jaronoff97 requested a review from a team August 27, 2024 17:20
@jaronoff97
Copy link
Contributor Author

this changes to use it for RBAC parsing, hoping to delete the old stuff in a different PR.

Copy link
Contributor

@swiatekm swiatekm left a comment

Choose a reason for hiding this comment

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

I'm not that familiar with the component parsing code, so I mostly limited my review to naming and whether the abstractions make sense to me in a vacuum. The test code looks good to be at a glance, but I'd feel more assured if someone with more expertise reviewed it before merging.

internal/components/builder.go Outdated Show resolved Hide resolved
internal/components/builder.go Outdated Show resolved Hide resolved
internal/components/component.go Outdated Show resolved Hide resolved
internal/components/multi_endpoint.go Outdated Show resolved Hide resolved
internal/components/generic_parser.go Outdated Show resolved Hide resolved
@jaronoff97 jaronoff97 merged commit f2a8cf2 into open-telemetry:main Sep 16, 2024
33 checks passed
@jaronoff97 jaronoff97 deleted the improve-rbac-parsing branch September 16, 2024 14:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Skip Changelog PRs that do not require a CHANGELOG.md entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants