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

Adding support for unidirectional contracts #95

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

robvand
Copy link

@robvand robvand commented May 30, 2024

Adding support for unidirectional contracts in the aci_contracts module.

Modified subject attributes to include:
apply_both_directions (toggling this knob will force recreation of the subject)
reverse_filter_ports (set to false/no if apply_both_directions is false)
service_graph_consumer_to_provider
service_graph_provider_to_consumer

Modified filter to subject attributes to include:
directions (provider_to_consumer, consumer_to_provider)

@robvand
Copy link
Author

robvand commented May 30, 2024

closes #94

Copy link
Contributor

@andbyrne andbyrne left a comment

Choose a reason for hiding this comment

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

My review is out of date. I will re-review once the code has been updated to align with final schema decisions.

priority = try(filter.priority, local.defaults.apic.tenants.contracts.subjects.filters.priority)
log = try(filter.log, local.defaults.apic.tenants.contracts.subjects.filters.log)
no_stats = try(filter.no_stats, local.defaults.apic.tenants.contracts.subjects.filters.no_stats)
directions = try(filter.directions, [])
Copy link
Contributor

Choose a reason for hiding this comment

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

Change to direction = try(subject.apply_both_directions, local.defaults.apic.tenants.contracts.apply_both_directions) ? null : filter.direction

This will throw an error if someone disables apply both directions but fails to set a direction in the filter.

name = "${subject.name}${local.defaults.apic.tenants.contracts.subjects.name_suffix}"
alias = try(subject.alias, "")
description = try(subject.description, "")
apply_both_directions = try(subject.apply_both_directions, true)
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't hardcode true, use the default value local.defaults.apic.tenants.contracts.apply_both_directions

alias = try(subject.alias, "")
description = try(subject.description, "")
apply_both_directions = try(subject.apply_both_directions, true)
reverse_filter_ports = try(subject.reverse_filter_ports, true)
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't hardcode true, use the default value local.defaults.apic.tenants.contracts.reverse_filter_ports

directives = join(",", concat(flt.log == true ? ["log"] : [], flt.no_stats == true ? ["no_stats"] : []))
priority = flt.priority
apply_both_directions = subj.apply_both_directions
directions = flt.directions
Copy link
Contributor

Choose a reason for hiding this comment

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

Rename directions to direction

action = flt.action
directives = join(",", concat(flt.log == true ? ["log"] : [], flt.no_stats == true ? ["no_stats"] : []))
priority = flt.priority
apply_both_directions = subj.apply_both_directions
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't need apply_both_directions here as a null value for direction will imply both directions.

}

resource "aci_rest_managed" "vzRsFiltAtt_provcons" {
for_each = { for filter in local.subj_filter_list : filter.id => filter if filter.apply_both_directions == false && contains(filter.directions, "provider_to_consumer") }
Copy link
Contributor

Choose a reason for hiding this comment

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

Make the condition if filter.direction == "provider_to_consumer"


resource "aci_rest_managed" "vzRsFiltAtt_consprov" {
for_each = { for filter in local.subj_filter_list : filter.id => filter if filter.apply_both_directions == false && contains(filter.directions, "consumer_to_provider") }
dn = "${aci_rest_managed.vzSubj_unidir[each.value.subj].dn}/intmnl/rsfiltAtt-${each.value.filter}"
Copy link
Contributor

Choose a reason for hiding this comment

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

Update aci_rest_managed.vzSubj_unidir to aci_rest_managed.vzSubj


resource "aci_rest_managed" "vzRsFiltAtt_provcons" {
for_each = { for filter in local.subj_filter_list : filter.id => filter if filter.apply_both_directions == false && contains(filter.directions, "provider_to_consumer") }
dn = "${aci_rest_managed.vzSubj_unidir[each.value.subj].dn}/outtmnl/rsfiltAtt-${each.value.filter}"
Copy link
Contributor

Choose a reason for hiding this comment

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

Update aci_rest_managed.vzSubj_unidir to aci_rest_managed.vzSubj


resource "aci_rest_managed" "vzRsInTermGraphAtt" {
for_each = { for subj in var.subjects : subj.name => subj if subj.service_graph_consumer_to_provider != null && subj.apply_both_directions == false }
dn = "${aci_rest_managed.vzSubj_unidir[each.key].dn}/intmnl/rsInTermGraphAtt"
Copy link
Contributor

Choose a reason for hiding this comment

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

Update aci_rest_managed.vzSubj_unidir to aci_rest_managed.vzSubj


resource "aci_rest_managed" "vzRsOutTermGraphAtt" {
for_each = { for subj in var.subjects : subj.name => subj if subj.service_graph_provider_to_consumer != null && subj.apply_both_directions == false }
dn = "${aci_rest_managed.vzSubj_unidir[each.key].dn}/outtmnl/rsOutTermGraphAtt"
Copy link
Contributor

Choose a reason for hiding this comment

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

Update aci_rest_managed.vzSubj_unidir to aci_rest_managed.vzSubj

@andbyrne andbyrne linked an issue Jun 21, 2024 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enhancement: Need support for uni-directional contracts
2 participants