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 nsg concatenation #233

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

vlappenbusch
Copy link
Contributor

NSG fields that use multiple inputs, such as SourceAddressPrefixes, should be mapped to a single field by concatenating and comma separating the array into one field. The comma separation was missed, making this SDK work not as intended for inputs with multiple values in the array.

This changes to use comma separation for array input fields, as well as the standardized string concatenation function through golang.

@vlappenbusch
Copy link
Contributor Author

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@nijosmsft
Copy link
Contributor

hi @vlappenbusch , is this still required? or did the operators changed their format?

@vlappenbusch
Copy link
Contributor Author

Yes, this should still be merged, I wasn't able to get testing put together for it yet but it should be fixed! I believe the operators have a workaround for now

@vlappenbusch
Copy link
Contributor Author

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@vlappenbusch
Copy link
Contributor Author

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@vlappenbusch
Copy link
Contributor Author

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

concatRule += prefix
}
wssdCloudNSGRule.SourceAddressPrefix = concatRule
wssdCloudNSGRule.SourceAddressPrefix = strings.Join(*rule.SourceAddressPrefixes, ",")
Copy link
Contributor

Choose a reason for hiding this comment

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

strings.Join(*rule.SourceAddressPrefixes, ",")

i remember you mentioned operators did some workarounds because of this bug, will those workarounds continue to work once we fix it? or do we need operators to change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I'll reach out to them to remove those fixes! I believe their fix was to concatenate the arrays themselves and to pass an array with one element that is a concatenated string, so that would still work. I can make sure to notify Arka and whoever else needs to know when this is merged.

Copy link
Contributor

Choose a reason for hiding this comment

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

ok, as this is low pri fix, let's hold on and dont merge until we are 100% sure. i would recommend sharing a private with them and testing. for context, our asz private preview is going to happen in 2408 we need to be extra careful with anything we merge.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just checked and this won't conflict. Their change is here https://dev.azure.com/msazure/msk8s/_git/moc-operator/pullrequest/10168753?_a=files&path=%2Fpkg%2Fhelper%2Fsecuritygrouphelper.go . The testing is already done and in PR as well, but it needs a version of moc-sdk-for-go before I ask for final reviews

mjethwa-msft
mjethwa-msft previously approved these changes Jul 18, 2024
@mjethwa-msft mjethwa-msft force-pushed the user/vlappenbusch/fixNsgConcatenation branch 2 times, most recently from 1556802 to 48ec90c Compare September 3, 2024 21:47
@mjethwa-msft mjethwa-msft force-pushed the user/vlappenbusch/fixNsgConcatenation branch from 48ec90c to 38bf694 Compare September 3, 2024 22:07
@mjethwa-msft
Copy link
Contributor

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

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.

3 participants