Add logging configuration to firewall #7

nnbu · 2023-08-31T22:16:06Z

Issue (aws-controllers-k8s/community#1553)

Description of changes:
Adds support for logging configuration to firwall spec

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

ack-prow · 2023-08-31T22:16:09Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: nnbu

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

ack-prow · 2023-08-31T22:16:15Z

Hi @nnbu. Thanks for your PR.

I'm waiting for a aws-controllers-k8s member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

a-hilaly · 2023-08-31T22:18:15Z

/ok-to-test

nnbu · 2023-09-01T05:24:29Z

/test networkfirewall-kind-e2e

a-hilaly

Hi @nnbu , looks like you need to update the commit hash in tests/e2e/requirements.txt it's the reason it's causing tests to fail

nnbu · 2023-09-01T16:59:34Z

/retest

nnbu · 2023-09-01T18:01:39Z

I will check the errors further. These are related to my code. The e2e tests run fine when I run it though. Need to see what is different in upstream.

nnbu · 2023-09-01T19:35:03Z

/test networkfirewall-kind-e2e

a-hilaly · 2023-09-01T22:13:44Z

/test networkfirewall-kind-e2e

a-hilaly · 2023-09-01T22:14:39Z

@nnbu could you also add a recommended IAM policy? it will help with networkfirewall-recommended-policy-test

nnbu · 2023-09-01T22:38:49Z

yes. I have not gotten a chance to look into where and what to add regarding the policy. I will check that

a-hilaly · 2023-09-01T22:40:17Z

@nnbu we need a file like https://github.com/aws-controllers-k8s/route53resolver-controller/blob/main/config/iam/recommended-policy-arn

a-hilaly · 2023-09-01T22:40:51Z

/test networkfirewall-kind-e2e

nnbu · 2023-09-05T17:30:33Z

@nnbu we need a file like https://github.com/aws-controllers-k8s/route53resolver-controller/blob/main/config/iam/recommended-policy-arn

@a-hilaly Thank you. Tests are passing now.

nnbu · 2023-09-07T20:20:31Z

Gentle request to review when possible

a-hilaly

@nnbu Nice, thanks a lot!

I have few questions/comments bellow. Feel free to ping me for questions or discussing any point.

apis/v1alpha1/ack-generate-metadata.yaml

templates/common/sdk_file_end.go.tpl

pkg/resource/firewall/hook.go

test/e2e/requirements.txt

test/e2e/service_bootstrap.py

test/e2e/tests/test_references.pytest

test/e2e/tests/test_firewall.py

a-hilaly · 2023-09-07T22:49:29Z

/hold

a-hilaly · 2023-09-13T19:13:39Z

/test all

a-hilaly · 2023-09-13T19:24:35Z

pkg/resource/firewall/hook.go

+	logDestinationConfigMap := make(map[string]*svcapitypes.LogDestinationConfig)
+
+	for _, cfg := range logDestinationConfig {
+		logDestinationConfigMap[makeLogDestinationConfigMapKey(cfg)] = cfg
+	}


nit suggestion:

Suggested change

logDestinationConfigMap := make(map[string]*svcapitypes.LogDestinationConfig)

for _, cfg := range logDestinationConfig {

logDestinationConfigMap[makeLogDestinationConfigMapKey(cfg)] = cfg

}

// since we only want to use this map to check for object existance

logDestinationConfigMap := make(map[string]*struct{})

for _, cfg := range logDestinationConfig {

logDestinationConfigMap[makeLogDestinationConfigMapKey(cfg)] = nil

}

I am using the map value 'cfg' in compareLoggingDestinationConfigs function, so we can not make value of the map as nil.

Let me explain the purpose of this map.

In compareLoggingDestinationConfigs() , we have to compare arrays of type []*svcapitypes.LogDestinationConfig. LogDestinationConfig in itself is complex type which has a map (LogDestination) as one of its fields. This makes comparison of these 2 arrays difficult.

So, to compare these arrays, I convert the array into a map, because map comparisons are easier to handle based on map keys. So, to convert each array object into a map entry, we need to have a key corresponding to that array object. makeLogDestinationConfigMapKey creates this key from each array object. The value associated with this key is the array object itself, so that it is easier to retrieve the array object.

So, in summary,

makeLogDestinationConfigMapKey gives unique key for each array entry of type *svcapitypes.LogDestinationConfig.

makeLogDestinationConfigMap creates map with its key provided by makeLogDestinationConfigMapKey and its value as the array entry of type svcapitypes.LogDestinationConfig

In compareLoggingDestinationConfigs , we create 2 maps. One that represents what is desired logging configuration and one that represents what is existing logging configuration.
Now that, we have 2 maps, we iterate over desired map and check if the keys are present in latest map. If they are not present, we append the value of that map (which is *svcapitypes.LogDestinationConfig) into the list of configs to be added. Same case to determine list of configs to be deleted.

I understand the problem, this is something we've seen with other APIs, but had to handle with some custom comparison/update functions. But in your case looks like it's a bit different.
I'm going to clone your branch and play a little bit with the implementation exploring the complexity of this case.

Let me know if you got a chance to check more on this one.

a-hilaly · 2023-09-13T21:11:26Z

pkg/resource/firewall/hook.go

+	keys := make([]string, 0, len(cfg.LogDestination))
+	for k := range cfg.LogDestination {
+		keys = append(keys, k)
+	}
+	sort.Strings(keys)
+
+	key := "LogDestination:"
+	for _, k := range keys {
+		v := *cfg.LogDestination[k]
+		key += fmt.Sprintf("%s:%s,", k, v)
+	}
+
+	if cfg.LogDestinationType != nil {
+		key += fmt.Sprintf("LogDestinationType:%s,", *cfg.LogDestinationType)
+	}
+	if cfg.LogType != nil {
+		key += fmt.Sprintf("LogType:%s", *cfg.LogType)
+	}
+	return key


My two cents: to avoid building strings manually with hard coded stuff, we can use json.Marshal to transform the map in into a "sorted" map - checkout https://go.dev/src/encoding/json/encode.go#L344

We dont really need to 'sort' the map here. So, I am not sure how I can make use of marshaling here. I explained the logic of compareLoggingDestinationConfigs and associated functions below. Please let me know if you have suggestions.

But i believe that you're calling sort.Strings(keys) to have deterministic output.. because of the hash map random access pattern... right? the idea was instead of manually building your "string" that is just a way of representating of the object, how about:

b1, err := json.Marshall(cfg1) b2, err := json.Marshall(cfg2)

then you case use string(b1), string(b2) as a key for the map discussed below

Thank you for the suggestion. I made a change to use json.Marshall

test/e2e/conftest.py

test/e2e/resources/firewall.yaml

nnbu · 2023-09-18T21:08:49Z

@a-hilaly Please let me know if any additional information is needed from my end.

nnbu · 2023-09-25T21:47:26Z

gentle reminder to review this one

pkg/resource/firewall/hook.go

a-hilaly · 2023-09-26T00:00:48Z

pkg/resource/firewall/hook.go

+	keys := make([]string, 0, len(cfg.LogDestination))
+	for k := range cfg.LogDestination {
+		keys = append(keys, k)
+	}
+	sort.Strings(keys)
+
+	key := "LogDestination:"
+	for _, k := range keys {
+		v := *cfg.LogDestination[k]
+		key += fmt.Sprintf("%s:%s,", k, v)
+	}
+
+	if cfg.LogDestinationType != nil {
+		key += fmt.Sprintf("LogDestinationType:%s,", *cfg.LogDestinationType)
+	}
+	if cfg.LogType != nil {
+		key += fmt.Sprintf("LogType:%s", *cfg.LogType)
+	}
+	return key


But i believe that you're calling sort.Strings(keys) to have deterministic output.. because of the hash map random access pattern... right? the idea was instead of manually building your "string" that is just a way of representating of the object, how about:

b1, err := json.Marshall(cfg1) b2, err := json.Marshall(cfg2)

then you case use string(b1), string(b2) as a key for the map discussed below

a-hilaly · 2023-09-26T00:09:24Z

pkg/resource/firewall/hook.go

+	logDestinationConfigMap := make(map[string]*svcapitypes.LogDestinationConfig)
+
+	for _, cfg := range logDestinationConfig {
+		logDestinationConfigMap[makeLogDestinationConfigMapKey(cfg)] = cfg
+	}


I understand the problem, this is something we've seen with other APIs, but had to handle with some custom comparison/update functions. But in your case looks like it's a bit different.
I'm going to clone your branch and play a little bit with the implementation exploring the complexity of this case.

a-hilaly · 2023-09-26T00:10:33Z

@nnbu I didn't forget about this PR, i will experiment few things and get back to you..

Issue (aws-controllers-k8s/community#1553) Description of changes: Adds support for logging configuration to firwall spec By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

nnbu · 2023-10-30T16:52:13Z

@a-hilaly I made the change to use b1, err := json.Marshall(cfg1).
Do you mind reviewing this one please?

ack-bot · 2024-04-27T19:11:32Z

Issues go stale after 180d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 60d of inactivity and eventually close.
If this issue is safe to close now please do so with /close.
Provide feedback via https://github.com/aws-controllers-k8s/community.
/lifecycle stale

ack-prow · 2024-08-30T17:32:28Z

@nnbu: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
networkfirewall-verify-attribution	`6257eeb`	link	true	`/test networkfirewall-verify-attribution`

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

ack-prow bot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Aug 31, 2023

ack-prow bot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Aug 31, 2023

a-hilaly reviewed Sep 1, 2023

View reviewed changes

nnbu force-pushed the logging-config-1 branch from ea18a4e to f07bb81 Compare September 1, 2023 16:58

nnbu force-pushed the logging-config-1 branch from f07bb81 to 3dbd2f6 Compare September 1, 2023 19:32

nnbu force-pushed the logging-config-1 branch from 3dbd2f6 to d231327 Compare September 5, 2023 16:43

a-hilaly reviewed Sep 7, 2023

View reviewed changes

ack-prow bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 7, 2023

nnbu force-pushed the logging-config-1 branch 3 times, most recently from 4d045bf to 606428a Compare September 9, 2023 05:11

a-hilaly reviewed Sep 13, 2023

View reviewed changes

nnbu force-pushed the logging-config-1 branch from 606428a to 4d8e1d5 Compare September 14, 2023 05:15

a-hilaly reviewed Sep 26, 2023

View reviewed changes

nnbu force-pushed the logging-config-1 branch from 4d8e1d5 to 542a518 Compare October 25, 2023 22:36

Add logging configuration to firewall

6257eeb

Issue (aws-controllers-k8s/community#1553) Description of changes: Adds support for logging configuration to firwall spec By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

nnbu force-pushed the logging-config-1 branch from 542a518 to 6257eeb Compare October 25, 2023 23:20

ack-prow bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Apr 27, 2024

a-hilaly removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jun 4, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add logging configuration to firewall #7

Add logging configuration to firewall #7

nnbu commented Aug 31, 2023

ack-prow bot commented Aug 31, 2023

ack-prow bot commented Aug 31, 2023

a-hilaly commented Aug 31, 2023

nnbu commented Sep 1, 2023

a-hilaly left a comment

nnbu commented Sep 1, 2023

nnbu commented Sep 1, 2023

nnbu commented Sep 1, 2023

a-hilaly commented Sep 1, 2023

a-hilaly commented Sep 1, 2023

nnbu commented Sep 1, 2023

a-hilaly commented Sep 1, 2023

a-hilaly commented Sep 1, 2023

nnbu commented Sep 5, 2023

nnbu commented Sep 7, 2023

a-hilaly left a comment

a-hilaly commented Sep 7, 2023

a-hilaly commented Sep 13, 2023

a-hilaly Sep 13, 2023

nnbu Sep 14, 2023

a-hilaly Sep 26, 2023

nnbu Oct 25, 2023

a-hilaly Sep 13, 2023

nnbu Sep 14, 2023

a-hilaly Sep 26, 2023

nnbu Oct 25, 2023

nnbu commented Sep 18, 2023

nnbu commented Sep 25, 2023

a-hilaly Sep 26, 2023

a-hilaly Sep 26, 2023

a-hilaly commented Sep 26, 2023

nnbu commented Oct 30, 2023

ack-bot commented Apr 27, 2024

ack-prow bot commented Aug 30, 2024

Add logging configuration to firewall #7

Are you sure you want to change the base?

Add logging configuration to firewall #7

Conversation

nnbu commented Aug 31, 2023

ack-prow bot commented Aug 31, 2023

ack-prow bot commented Aug 31, 2023

a-hilaly commented Aug 31, 2023

nnbu commented Sep 1, 2023

a-hilaly left a comment

Choose a reason for hiding this comment

nnbu commented Sep 1, 2023

nnbu commented Sep 1, 2023

nnbu commented Sep 1, 2023

a-hilaly commented Sep 1, 2023

a-hilaly commented Sep 1, 2023

nnbu commented Sep 1, 2023

a-hilaly commented Sep 1, 2023

a-hilaly commented Sep 1, 2023

nnbu commented Sep 5, 2023

nnbu commented Sep 7, 2023

a-hilaly left a comment

Choose a reason for hiding this comment

a-hilaly commented Sep 7, 2023

a-hilaly commented Sep 13, 2023

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

nnbu commented Sep 18, 2023

nnbu commented Sep 25, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

a-hilaly commented Sep 26, 2023

nnbu commented Oct 30, 2023

ack-bot commented Apr 27, 2024

ack-prow bot commented Aug 30, 2024