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

filter bad characters from label keys when creating tags list #814

Merged
merged 19 commits into from
Apr 1, 2020
Merged

filter bad characters from label keys when creating tags list #814

merged 19 commits into from
Apr 1, 2020

Conversation

frodopwns
Copy link
Contributor

closes #805

What this PR does / why we need it:
Azure doesn't allow some characters in tag keys that Kube allows in label keys. This PR
creates a function that filters the disallowed characters and returns the rest of the labels in the format expected by Azure for tags.

This PR also implements the function in the Azure SQL Server controller.

@frodopwns frodopwns requested a review from melonrush13 March 25, 2020 17:19
@melonrush13
Copy link
Contributor

Should we create another ticket to add this to the rest of the controllers which use tags?

@melonrush13
Copy link
Contributor

Want to make sure I am understanding the expected performance correctly for issue #805
When a user deploys a sql server with some improper syntax, we still want to successfully deploy the sql server, but ignore their incorrect syntax labels? Yes?

issues := [][]string{}
for k, v := range in {
if strings.ContainsAny(k, "<>%/?\\") {
issues = append(issues, []string{k, "contains a character not allowed in Azure tags"})
Copy link
Contributor

Choose a reason for hiding this comment

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

Right now the user never sees "contains a character not allowed in Azure tags". Is that okay? Do we want to "log" this somewhere? I only see this message from Kubernetes The AzureSqlServer "sqlserver-sample-777-meltwo" is invalid: metadata.labels: Invalid value: "value/one": a valid label must be an empty string or consist of alphanumeric characters, '-', '_' or '.', and must start and end with an alphanumeric character (e.g. 'MyValue', or 'my_value', or '12345', regex used for validation is '(([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9])?')) Which is how it performs on master too

Copy link
Contributor Author

Choose a reason for hiding this comment

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

some characters even Kube won't let you put in labels :-)

try something like "example.com/test": "yay"

I added the issues for when we decide what (if anything) to do with them. Right now they are ignored.

Copy link
Contributor

Choose a reason for hiding this comment

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

:) I see. That makes a lot mores sense! Thanks!

@melonrush13
Copy link
Contributor

Should we create another ticket to add this to the rest of the controllers which use tags?

Made 3 tix - one for keyvault (#828), cosmos(#827), and redis (#826)
Hope that's cool - feel free to change as you feel need. :)

@frodopwns
Copy link
Contributor Author

Should we create another ticket to add this to the rest of the controllers which use tags?

yeah, that is probably a good idea

@frodopwns
Copy link
Contributor Author

Want to make sure I am understanding the expected performance correctly for issue #805
When a user deploys a sql server with some improper syntax, we still want to successfully deploy the sql server, but ignore their incorrect syntax labels? Yes?

if characters that Azure won't accept in tag keys exist in a certain label it just won't be used. We can talk about whether to log it or what.

@melonrush13
Copy link
Contributor

melonrush13 commented Mar 26, 2020

Want to make sure I am understanding the expected performance correctly for issue #805
When a user deploys a sql server with some improper syntax, we still want to successfully deploy the sql server, but ignore their incorrect syntax labels? Yes?

if characters that Azure won't accept in tag keys exist in a certain label it just won't be used. We can talk about whether to log it or what.

Ok! Thanks for clarifying. Right now, when I make a sqlserver with 1 valid tag and 1 invalid tag it does not currently create at all. I get the error message on creation, and then when i do an -o yaml on that name, it says "not found".

Copy link
Contributor Author

@frodopwns frodopwns left a comment

Choose a reason for hiding this comment

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

had a call with some users and they would like to translate the characters to something acceptable instead of dropping the labels...seems reasonable to me

@frodopwns frodopwns requested a review from melonrush13 March 30, 2020 18:21
@jananivMS jananivMS self-requested a review March 31, 2020 22:37
jananivMS
jananivMS previously approved these changes Mar 31, 2020
Copy link
Contributor

@jananivMS jananivMS left a comment

Choose a reason for hiding this comment

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

Looks good. Added a suggestion for including ? in the replace call.

newK := k
value := v
if strings.ContainsAny(k, "<>%/?\\") {
newK = ReplaceAny(k, []string{"<", ">", "%", "/", "\\\\"})
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
newK = ReplaceAny(k, []string{"<", ">", "%", "/", "\\\\"})
newK = ReplaceAny(k, []string{"<", ">", "%", "/", "?", "\\\\"})

Copy link
Contributor

@jananivMS jananivMS left a comment

Choose a reason for hiding this comment

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

lgtm

@frodopwns frodopwns merged commit 23625bd into Azure:master Apr 1, 2020
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.

Task/Bug: some characters allowed in Kube labels are not allowed in Azure tags
3 participants