Fix bug where coredns 1.8.3 needs more permissions #130
Conversation
eks/sync.go
Outdated
| corednsConfigMap, err := getCorednsConfigMap(clientset) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| // Need to check config for backwards incompatibility if updating to version >= 1.7.0. The keyword upstream was |
There was a problem hiding this comment.
NIT: I would probably word this comment as
The keyword "upstream" or `upstream`
to be clear that upstream is the keyword.
| // Need to check config for backwards incompatibility if updating to version >= 1.7.0. The keyword upstream was | ||
| // removed in 1.7 series of coredns, but is used in earlier versions. | ||
| compareVal, err := semverStringCompare(coreDNSVersion, "1.7.0-eksbuild.1") | ||
| compareVal170, err := semverStringCompare(coreDNSVersion, "1.7.0-eksbuild.1") | ||
| if err != nil { | ||
| return err | ||
| } | ||
| // Looking for 1 or 0 here, since we want to know when coreDNSVersion is >= 1.7.0 |
There was a problem hiding this comment.
NIT: Since annotated versions are considered <, the comparison is actually against 1.7.0-eksbuild.1, which is < 1.7.0. I don't know if it really matters in this comment, but it strikes me as slightly inaccurate. Do I understand correctly?
There was a problem hiding this comment.
I think it could be argued that it doesn't matter for the sake of this comment, since 1.7.0-eksbuild.1 is still 1.7.0 colloquially.
There was a problem hiding this comment.
This doesn't really matter, because all the EKS coredns versions are always published with the annotation, so the comment is accurate in the context of EKS.
There was a problem hiding this comment.
LGTM! Left minor nits, feel free to take or leave. Assuming tests pass in the other PR, 👍 .
NIT: On the whole, I wonder about these updates scaling. I like that these two new update...Compatibility functions are named for both the version and the thing being updated. I wonder if it is easier in the long run to process compatibility updates for coredns in a slightly different way, where maybe, based on a single semverStringCompare, you run all the updates that are needed. Again, definitely not critical now.
I don't think we should worry too much about this. EKS is only going to maintain support for a handful of kubernetes versions (as evidenced by the fact that they continue to roll off the oldest version everytime they add support for a new version). And the number of versions that need special treatment will be even less than that. Furthermore, this command is going to be deprecated real soon when we switch to using managed add-ons, which will encapsulate this logic at the AWS API level. |
Can you elaborate what you are thinking here? I don't think I fully understand what you are proposing here. I'm having a hard time figuring out how you can reduce the checks down to a single |
|
Thanks for the review! I'm waiting on the integration testing build to pass before merging this in. |
Good question. I was thinking the way to know which (how many) updates to apply would be derived from the difference in versions. 🤔 I am not sure if that's possible. I think the |
Wouldn't that be much more confusing as a maintainer? E.g., you now have to do the math to update the |
|
Ok integration testing branch passed, so will go ahead and merge this in! Thanks for the review! |
Fixes #129
EKS integration testing PR is https://github.com/gruntwork-io/terraform-aws-eks/pull/331