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

fix(networkfirewall): fixed import for networkfirewall resources #661

Merged

Conversation

haarchri
Copy link
Member

@haarchri haarchri commented Apr 6, 2023

Description of your changes

  1. fix import for the following networkfirewall resources:
    aws_networkfirewall_firewall_policy
    aws_networkfirewall_rule_group

2)implement missing ref/selector for ruleGroup in aws_networkfirewall_firewall_policy
3)add more complex example for aws_networkfirewall_firewall_policy and aws_networkfirewall_rule_group

Fixes #658
Fixes #657

I have:

  • Run make reviewable test to ensure this PR is ready for review.

How has this code been tested

kubectl apply -f firewallpolicy-complex.yaml
firewallpolicy.networkfirewall.aws.upbound.io/example-default-policy created
rulegroup.networkfirewall.aws.upbound.io/example-forward-all created
rulegroup.networkfirewall.aws.upbound.io/example-allow-domainlist created
rulegroup.networkfirewall.aws.upbound.io/example-deny created
NAME                                                                   READY   SYNCED   EXTERNAL-NAME            AGE
firewallpolicy.networkfirewall.aws.upbound.io/example-default-policy   True    True     example-default-policy   3m17s

NAME                                                                READY   SYNCED   EXTERNAL-NAME              AGE
rulegroup.networkfirewall.aws.upbound.io/example-allow-domainlist   True    True     example-allow-domainlist   3m17s
rulegroup.networkfirewall.aws.upbound.io/example-deny               True    True     example-deny               3m17s
rulegroup.networkfirewall.aws.upbound.io/example-forward-all        True    True     example-forward-all        3m17s

updated deletionPolicy: Orphan

kubectl delete -f firewallpolicy-complex.yaml 
firewallpolicy.networkfirewall.aws.upbound.io "example-default-policy" deleted
rulegroup.networkfirewall.aws.upbound.io "example-forward-all" deleted
rulegroup.networkfirewall.aws.upbound.io "example-allow-domainlist" deleted
rulegroup.networkfirewall.aws.upbound.io "example-deny" deleted
kubectl apply -f firewallpolicy-complex.yaml
firewallpolicy.networkfirewall.aws.upbound.io/example-default-policy created
rulegroup.networkfirewall.aws.upbound.io/example-forward-all created
rulegroup.networkfirewall.aws.upbound.io/example-allow-domainlist created
rulegroup.networkfirewall.aws.upbound.io/example-deny created
NAME                                                                   READY   SYNCED   EXTERNAL-NAME            AGE
firewallpolicy.networkfirewall.aws.upbound.io/example-default-policy   True    True     example-default-policy   1m12s

NAME                                                                READY   SYNCED   EXTERNAL-NAME              AGE
rulegroup.networkfirewall.aws.upbound.io/example-allow-domainlist   True    True     example-allow-domainlist   1m12s
rulegroup.networkfirewall.aws.upbound.io/example-deny               True    True     example-deny               1m12s
rulegroup.networkfirewall.aws.upbound.io/example-forward-all        True    True     example-forward-all        1m12s

@haarchri
Copy link
Member Author

haarchri commented Apr 6, 2023

cannot add breaking change to this PR :/
checkout: https://github.com/upbound/provider-aws/actions/runs/4630635501/jobs/8192470088?pr=661#step:4:15

@haarchri
Copy link
Member Author

haarchri commented Apr 6, 2023

/test-examples="examples/networkfirewall/firewall.yaml"

@haarchri
Copy link
Member Author

haarchri commented Apr 6, 2023

/test-examples="examples/networkfirewall/firewallpolicy-complex.yaml"

Copy link
Collaborator

@ytsarev ytsarev left a comment

Choose a reason for hiding this comment

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

Thanks a lot for the thorough testing. Left couple of comments regarding external name configuration

config/externalname.go Outdated Show resolved Hide resolved
config/externalname.go Outdated Show resolved Hide resolved
@haarchri
Copy link
Member Author

haarchri commented Apr 6, 2023

@ulucinar any idea for the issue for import ? the only thing which helped was to switch the identifier - great to hear your feedback ;)

Copy link
Collaborator

@ulucinar ulucinar left a comment

Choose a reason for hiding this comment

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

Hi @haarchri,
Thank you for addressing these issues. I'm posting some review comments to discuss.

I will also investigate why the old configuration is broken and leave a comment in the discussion.

config/networkfirewall/config.go Outdated Show resolved Hide resolved
config/externalname.go Outdated Show resolved Hide resolved
examples/networkfirewall/firewallpolicy-complex.yaml Outdated Show resolved Hide resolved
examples/networkfirewall/firewallpolicy-complex.yaml Outdated Show resolved Hide resolved
examples/networkfirewall/firewallpolicy-complex.yaml Outdated Show resolved Hide resolved
examples/networkfirewall/firewallpolicy-complex.yaml Outdated Show resolved Hide resolved
Copy link
Collaborator

@ulucinar ulucinar left a comment

Choose a reason for hiding this comment

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

Hi @haarchri,
After the root cause analysis, I'm leaving some more comments for us to discuss. If we do not switch to config.IdentifierFromProvider, then we will also need to change the extractors in the newly introduced referencer configuration. Currently, those new referencer configurations are relying on the rule group being configured with config.IdentifierFromProvider. We may use the common.PathARNExtractor to extract the ARN of a rule group resource.

config/externalname.go Outdated Show resolved Hide resolved
config/externalname.go Outdated Show resolved Hide resolved
Christopher Paul Haar added 2 commits April 27, 2023 21:43
… and rulegroup and implemented more ref/selectors in firewallpolicy for embedded rulegroup

Signed-off-by: Christopher Paul Haar <christopherpaul.haar@dkb.de>
… and rulegroup and implemented more ref/selectors in firewallpolicy for embedded rulegroup

Signed-off-by: Christopher Paul Haar <christopherpaul.haar@dkb.de>
@haarchri haarchri force-pushed the fix/networkfirewall-scenario branch from 91f315c to 244a032 Compare April 28, 2023 07:24
@haarchri
Copy link
Member Author

@ulucinar we can remove the breaking-change label then

@haarchri
Copy link
Member Author

/test-examples="examples/networkfirewall/firewallpolicy-complex.yaml"

Copy link
Collaborator

@ulucinar ulucinar left a comment

Choose a reason for hiding this comment

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

Thank you @haarchri, lgtm.

@ulucinar ulucinar merged commit 80a236d into crossplane-contrib:main Apr 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants