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

Create a filter implementation for K8s runtime.Objects #2631

Merged
merged 6 commits into from Mar 13, 2024

Conversation

michaelmdresser
Copy link
Contributor

What does this PR change?

  • Introduces a new filter parser and MatchCompiler implementation targeting the runtime.Object type in the K8s library
  • Moves filter field strings to a common package to avoid duplication and drift in future filter implementations

Does this PR relate to any other PRs?

  • Not in Opencost (will be used by a downstream dependency to use Opencost-style filter strings for a new purpose)

How will this PR impact users?

  • N/A

Does this PR address any GitHub or Zendesk issues?

  • N/A

How was this PR tested?

  • New unit tests

Does this PR require changes to documentation?

  • No

Copy link

vercel bot commented Mar 12, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
opencost ✅ Ready (Inspect) Visit Preview 💬 Add feedback Mar 13, 2024 7:50pm

@michaelmdresser michaelmdresser marked this pull request as ready for review March 12, 2024 22:03
@mbolt35
Copy link
Contributor

mbolt35 commented Mar 12, 2024

One very explicit thing you will need is to register the fields in the ops package either by adding it directly here:

var defaultFieldByType = map[string]any{
typeutil.TypeOf[allocation.AllocationField](): allocation.DefaultFieldByName,
typeutil.TypeOf[asset.AssetField](): asset.DefaultFieldByName,
typeutil.TypeOf[cloudcost.CloudCostField](): cloudcost.DefaultFieldByName,
// typeutil.TypeOf[containerstats.ContainerStatsField](): containerstats.DefaultFieldByName,
}

(this is likely the best choice -- just replace the // typeutil.TypeOf[containerstats.ContainerStatsField](): containerstats.DefaultFieldByName, line - heheheh)

If the fields are added outside of the opencost repo, you can use this in an init:

func RegisterDefaultFieldLookup[T ~string](lookup func(T) *ast.Field) {

@michaelmdresser
Copy link
Contributor Author

TIL! Done, that point about stuff outside of the Opencost repo is neat as heck

Comment on lines +12 to +17
FieldNamespace K8sObjectField = K8sObjectField(fieldstrings.FieldNamespace)
FieldControllerKind K8sObjectField = K8sObjectField(fieldstrings.FieldControllerKind)
FieldControllerName K8sObjectField = K8sObjectField(fieldstrings.FieldControllerName)
FieldPod K8sObjectField = K8sObjectField(fieldstrings.FieldPod)
FieldLabel K8sObjectField = K8sObjectField(fieldstrings.FieldLabel)
FieldAnnotation K8sObjectField = K8sObjectField(fieldstrings.FieldAnnotation)
Copy link
Contributor

Choose a reason for hiding this comment

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

Did go force you to add the type cast when you referred to the fieldstrings constants?

ie:

const FieldNamespace K8sObjectField = "namespace"

is fine, but

const FieldNamespace K8sObjectField = fieldstrings.FieldNamespace

barks at you that they're not the same type?

Bah

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, it barks; I'm glad it does, otherwise these enum-like things would be pretty useless.

// valid left-hand comparators
var k8sObjectFilterFields []*ast.Field = []*ast.Field{
ast.NewField(FieldNamespace),
ast.NewField(FieldControllerName, ast.FieldAttributeNilable),
Copy link
Contributor

Choose a reason for hiding this comment

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

Just wanted to point out (if you haven't noted field attributes yet) that ast.FieldAttributeNilable (or any attribute that might be needed) on a field is simply metadata that can be accessed in the compiler or code generator to help alleviate any special cases that may exist.

Copy link
Contributor

@mbolt35 mbolt35 Mar 13, 2024

Choose a reason for hiding this comment

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

Attributes are just bitflags (sorry, I think I'm showing my age), so they're not super expansive, but likely more space than we'll ever need. I think Nilable starts at the 5th bit, and we're using an int, so it's expandable to another 27 custom attributes 🤷

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh very neat, I just copied whatever Allocation was using for consistency; I think it also makes sense here.

Copy link
Contributor

@mbolt35 mbolt35 left a comment

Choose a reason for hiding this comment

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

LGTM

Signed-off-by: Michael Dresser <michaelmdresser@gmail.com>
Signed-off-by: Michael Dresser <michaelmdresser@gmail.com>
Signed-off-by: Michael Dresser <michaelmdresser@gmail.com>
Signed-off-by: Michael Dresser <michaelmdresser@gmail.com>
Signed-off-by: Michael Dresser <michaelmdresser@gmail.com>
Signed-off-by: Michael Dresser <michaelmdresser@gmail.com>
Copy link

sonarcloud bot commented Mar 13, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

@michaelmdresser michaelmdresser merged commit b55dbfb into develop Mar 13, 2024
10 checks passed
@michaelmdresser michaelmdresser deleted the mmd/k8s-filter branch March 13, 2024 19:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants