feat: add v1alpha3 api with RuntimeSpec configuration #922

ashnamehrotra · 2023-11-30T21:49:31Z

What this PR does / why we need it:
Part 1 of adding RuntimeSpec configuration. Adds v1alpha3 api with RuntimeSpec field and relevant conversion functions.

Which issue(s) this PR fixes (optional, using fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when the PR gets merged):
Fixes #

Special notes for your reviewer:

Signed-off-by: ashnamehrotra <[email protected]>

…erridden Signed-off-by: ashnamehrotra <[email protected]>

Signed-off-by: ashnamehrotra <[email protected]>

pmengelbert · 2023-12-04T20:25:27Z

api/v1alpha3/config/config.go

+	cfg *v1alpha3.EraserConfig
+}
+
+func (m *Manager) Read() (v1alpha3.EraserConfig, error) {


I believe you can remove these Read and Update methods and the NewManager function. Only the unversioned package needs those. For example, the corresponding functions from the v1alpha2/config package are not actually called anywhere.

I think the functions aren't even needed for the conversion generation either. You can remove these, and the corresponding ones in v1alpha1 and v1alpha2 as well.

api/v1alpha3/eraserconfig_types.go

api/v1alpha3/custom_conversions.go

api/v1alpha3/eraserconfig_types.go

pmengelbert · 2023-12-04T20:35:05Z

api/v1alpha3/eraserconfig_types.go

+	return rs, nil
+}
+
+func (td *Duration) UnmarshalJSON(b []byte) error {


I'd also like to have a unit test to prevent this from regressing. The unmarshaling is important because it catches errors, and we need to have tests to prove it fails when it is supposed to and doesn't when it's not.

pmengelbert · 2023-12-04T20:36:44Z

api/v1alpha3/custom_conversions.go

+// TODO: change this to use unversioned.RuntimeSpec when unversioned is updated
+//
+//nolint:revive
+func autoConvert_unversioned_Runtime_To_v1alpha3_RuntimeSpec(in *unversioned.Runtime, out *RuntimeSpec, _ conversion.Scope) error {


This shouldn't be named autoConvert..., since it's manually written, right?

pmengelbert

This PR isn't quite complete. We need to replace the unversioned.EraserConfig with the v1alpha3 version. The unversioned should always be the latest stable version. Since none of our EraserConfig versions are stable, it should always just be the latest.

This will have effects on some of the code here, since it won't be necessary to have conversion functions between v1alpha3 and unversioned. But you will need converters for 1 -> unversioned and 2 -> unversioned.

Signed-off-by: ashnamehrotra <[email protected]>

pmengelbert · 2023-12-04T22:32:54Z

On second thought, it's best to keep the unversioned api at v1alpha2, and to switch it to v1alpha3 in the future when it's properly wired up.

Signed-off-by: ashnamehrotra <[email protected]>

pmengelbert · 2023-12-05T14:05:37Z

api/v1alpha3/eraserconfig_types.go

+func ConvertRuntimeToRuntimeSpec(r Runtime) (RuntimeSpec, error) {
+	var rs RuntimeSpec
+
+	switch rt := r; rt {


Just switch r here

Signed-off-by: ashnamehrotra <[email protected]>

pmengelbert · 2023-12-14T20:20:49Z

api/v1alpha3/eraserconfig_types.go

+type (
+	Duration       time.Duration
+	Runtime        string
+	RuntimeAddress string


Why the newtype for RuntimeAddress? Why not just use a string?

changed to string only Runtime has its own type

pmengelbert · 2023-12-14T20:23:44Z

api/v1alpha3/runtime_spec_test.go

+
+	tests := map[string]testCase{
+		"ValidContainerd": {
+			input:     []byte(`{"Name": "containerd", "Address": "unix:///run/containerd/containerd.sock"}`),


The keys should all be lowercase, i.e. name and not Name. If the test is passing as-is, it is a false positive and we need to find out why.

And by keys I mean the keys in the JSON byte string.

changed keys to lowercase! it says here it accepts case-insensitive match for the keys: https://pkg.go.dev/encoding/json#Unmarshal

pmengelbert · 2023-12-14T20:27:31Z

pkg/scanners/trivy/helpers.go

@@ -12,12 +12,14 @@ func loadConfig(filename string) (Config, error) {

 	b, err := os.ReadFile(filename)
 	if err != nil {
+		log.Info("LOADCONFIG unable to read file")


Maybe log.Error here, and remove the LOADCONFIG -- the logger should give us more info on where the error is occurring.

pmengelbert · 2023-12-14T20:30:54Z

api/v1alpha3/runtime_spec_test.go

+	for name, test := range tests {
+		t.Run(name, func(t *testing.T) {
+			var rs RuntimeSpec
+			err := rs.UnmarshalJSON(test.input)


I think we shouldn't call UnmarshalJSON directly but rather just err := json.Unmarshal(test.input, &rs). That's because json.Unmarshal will ultimately call rs.UnmarshalJSON. That's how it ends up getting used in our code, so that's what we should test.

Signed-off-by: ashnamehrotra <[email protected]>

pmengelbert

LGTM, thanks!

Signed-off-by: ashnamehrotra <[email protected]>

ashnamehrotra added 3 commits November 30, 2023 13:39

initial code to add v1alpha3 api with RuntimeSpec configuration

2157091

Signed-off-by: ashnamehrotra <[email protected]>

cleanup conversion file

295cd7c

Signed-off-by: ashnamehrotra <[email protected]>

Merge branch 'main' of github.com:Azure/eraser into add-runtime-sock

4fdf322

ashnamehrotra force-pushed the add-runtime-sock branch from 8e25e74 to 4fdf322 Compare December 1, 2023 00:00

ashnamehrotra requested a review from pmengelbert December 1, 2023 00:00

ashnamehrotra added 3 commits November 30, 2023 16:12

modify unversioned_RuntimeSpec to unversioned_Runtime for this PR

97fa922

Signed-off-by: ashnamehrotra <[email protected]>

move custom conversions to another file to prevent them from being ov…

e1ba0a4

…erridden Signed-off-by: ashnamehrotra <[email protected]>

add //nolint:unparam,varcheck above custom conversion functions

5d939a4

Signed-off-by: ashnamehrotra <[email protected]>

ashnamehrotra force-pushed the add-runtime-sock branch from ea576fd to 5d939a4 Compare December 1, 2023 21:23

ashnamehrotra added 3 commits December 1, 2023 14:03

try unsused

0471238

Signed-off-by: ashnamehrotra <[email protected]>

lint change unused scope variable to _

4430bbb

Signed-off-by: ashnamehrotra <[email protected]>

nolint:revive

cd806c5

Signed-off-by: ashnamehrotra <[email protected]>

pmengelbert reviewed Dec 4, 2023

View reviewed changes

api/v1alpha3/eraserconfig_types.go Outdated Show resolved Hide resolved

pmengelbert reviewed Dec 4, 2023

View reviewed changes

api/v1alpha3/custom_conversions.go Outdated Show resolved Hide resolved

pmengelbert reviewed Dec 4, 2023

View reviewed changes

api/v1alpha3/eraserconfig_types.go Show resolved Hide resolved

pmengelbert reviewed Dec 4, 2023

View reviewed changes

pmengelbert requested changes Dec 4, 2023

View reviewed changes

ashnamehrotra added 4 commits December 4, 2023 12:52

change auto in func name to manual for relevant funcs

0e7a8e7

Signed-off-by: ashnamehrotra <[email protected]>

remove unnecessary Runtime cast

5ab3676

Signed-off-by: ashnamehrotra <[email protected]>

remove Read, Update, NewManager funcs from v1alpha configs

c625811

Signed-off-by: ashnamehrotra <[email protected]>

remove unversioned TODOs

74fdb3a

Signed-off-by: ashnamehrotra <[email protected]>

ashnamehrotra added 2 commits December 4, 2023 22:49

fix RuntimeSpec unmarshall func

fa8f247

Signed-off-by: ashnamehrotra <[email protected]>

add unit tests

5d67b3d

Signed-off-by: ashnamehrotra <[email protected]>

pmengelbert reviewed Dec 5, 2023

View reviewed changes

switch r Runtime

81b2e2e

Signed-off-by: ashnamehrotra <[email protected]>

ashnamehrotra requested a review from pmengelbert December 5, 2023 22:58

change unversioned to v1alpha3 changes, modify relevant conversions

ddeaecb

Signed-off-by: ashnamehrotra <[email protected]>

ashnamehrotra force-pushed the add-runtime-sock branch from be2eb15 to ddeaecb Compare December 6, 2023 22:06

ashnamehrotra added 2 commits December 6, 2023 15:56

lint fixes

755f06a

Signed-off-by: ashnamehrotra <[email protected]>

add case for v1alpha3 apiversion

0409e51

Signed-off-by: ashnamehrotra <[email protected]>

ashnamehrotra force-pushed the add-runtime-sock branch from d3c6745 to 0409e51 Compare December 6, 2023 23:57

ashnamehrotra added 6 commits December 7, 2023 14:22

unmarshal into string values

442a1b7

Signed-off-by: ashnamehrotra <[email protected]>

debugging for CI errors

101b634

Signed-off-by: ashnamehrotra <[email protected]>

more debugging

4b0de32

Signed-off-by: ashnamehrotra <[email protected]>

update values files to v1alpha3

b4ef1d7

Signed-off-by: ashnamehrotra <[email protected]>

update manifest_staging/deploy/eraser.yaml

b0d8dce

Signed-off-by: ashnamehrotra <[email protected]>

update helm-empty-values.yaml

c8e2dc7

Signed-off-by: ashnamehrotra <[email protected]>

ashnamehrotra force-pushed the add-runtime-sock branch from 3fe226d to c8e2dc7 Compare December 12, 2023 02:44

pmengelbert reviewed Dec 14, 2023

View reviewed changes

ashnamehrotra added 4 commits December 14, 2023 12:55

runtime spec unit test changes

4c4ffed

Signed-off-by: ashnamehrotra <[email protected]>

change trivy log errors

bb0d846

Signed-off-by: ashnamehrotra <[email protected]>

remove RuntimeAddress type and use string

c34e194

Signed-off-by: ashnamehrotra <[email protected]>

Merge branch 'main' of github.com:Azure/eraser into add-runtime-sock

1f69c3f

pmengelbert approved these changes Dec 14, 2023

View reviewed changes

ashnamehrotra merged commit afb831b into eraser-dev:main Dec 14, 2023
169 of 170 checks passed

ashnamehrotra added a commit to ashnamehrotra/eraser that referenced this pull request Jan 25, 2024

feat: add v1alpha3 api with RuntimeSpec configuration (eraser-dev#922)

b6b0ea5

Signed-off-by: ashnamehrotra <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add v1alpha3 api with RuntimeSpec configuration #922

feat: add v1alpha3 api with RuntimeSpec configuration #922

ashnamehrotra commented Nov 30, 2023

pmengelbert Dec 4, 2023 •

edited

Loading

pmengelbert Dec 4, 2023

pmengelbert Dec 4, 2023

pmengelbert left a comment

pmengelbert commented Dec 4, 2023 •

edited

Loading

pmengelbert Dec 5, 2023

pmengelbert Dec 14, 2023

ashnamehrotra Dec 14, 2023

pmengelbert Dec 14, 2023

pmengelbert Dec 14, 2023

ashnamehrotra Dec 14, 2023

pmengelbert Dec 14, 2023

pmengelbert Dec 14, 2023

pmengelbert left a comment

feat: add v1alpha3 api with RuntimeSpec configuration #922

feat: add v1alpha3 api with RuntimeSpec configuration #922

Conversation

ashnamehrotra commented Nov 30, 2023

pmengelbert Dec 4, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pmengelbert left a comment

Choose a reason for hiding this comment

pmengelbert commented Dec 4, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pmengelbert left a comment

Choose a reason for hiding this comment

pmengelbert Dec 4, 2023 •

edited

Loading

pmengelbert commented Dec 4, 2023 •

edited

Loading