Add aks support #100
Add aks support #100
Conversation
|
Thanks for the PR! I'll take a look at it over the next few days. While I do so, can you update the version of |
|
so sorry, I got distracted and totally dropped the ball on reviewing this. I'll make it a priority for the next day or two. |
|
@mtougeron We have this change running for quite some time now and it seems to function stable. |
|
Yes, I've been really distracted as of late. I'm really sorry for not getting this taken care of. It's a top priority for me to get this merged and released in the next day or two. |
There was a problem hiding this comment.
This is great stuff. I really appreciate the addition. Again, I'm very sorry for how long this took me to review.
I have a few pieces of feedback, but the main thing of concern is the sanitization of the tags and how stripping the invalid characters can bypass other validation rules.
Otherwise, this is AWESOME!
aks.go
Outdated
| return nil, ErrAzureTooManyTags | ||
| } | ||
| for k, v := range tags { | ||
| k = sanitizeKeyForAzure(k) |
There was a problem hiding this comment.
After sanitizing the key it is possible that it could end up being a duplicate key. I'm on the fence as to whether it would be better to reject the tag if the key needs to be sanitized or if the tag created a duplicate. Which approach do you think is better?
There was a problem hiding this comment.
We use this in AWS and Azure so we don't really want to change the tags depending on the envrionment.
But i will add an error in case there are duplicated keys, this should also tackle your other comment concerning Kubernetes/Cluster: foo
aks.go
Outdated
| // remove forbidden characters | ||
| if strings.ContainsAny(s, `<>%&\?/`) { | ||
| for _, c := range `<>%&\?/` { | ||
| s = strings.ReplaceAll(s, string(c), "") |
There was a problem hiding this comment.
If the invalid characters are being removed from the key it is possible to bypass this logic https://github.com/mtougeron/k8s-pvc-tagger/blob/main/kubernetes.go#L327-L339 by creating a tag name of Kubernetes/Cluster: foo
There was a problem hiding this comment.
Its solved by rejecting duplicated tag keys during the sanitization.
README.md
Outdated
| @@ -117,6 +117,9 @@ gcloud iam roles create CustomDiskRole \ | |||
| --stage="GA" | |||
| ``` | |||
|
|
|||
| #### Azure rule | |||
| The default role `Tag Contributor` can be used to configure the access rights for the pvc-tagger. | |||
There was a problem hiding this comment.
Can you add a bit about how this only support Azure disk csi volumes?
There was a problem hiding this comment.
I've documented the behaviors hopefully this description will stay up to date in the future.
|
@boris-smidt-klarrio btw, if these changes are too much of a pain for you, because of how long I took to get to a review, I'm okay with merging as-is and creating an RC release of the image and making the changes myself. Just let me know what you'd prefer. |
|
@mtougeron I'll pull it to the end, there isn't any rush to this we are running an internal version of this image in the meantime. |
|
One minor change I've made after reading the GCP note i changed the behavior to replace the invalid characters in azure with _ as wel. |
Adding support for tagging pvc's in AKS.
We have tested this code internally to make sure it works.