-
Notifications
You must be signed in to change notification settings - Fork 966
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
[TK-1373] Update resource and data of 'kubernetes_(default_)service_account' to handle deprecated 'default_secret_name' in Kubernetes 1.24.0+ #1792
Conversation
ca74686
to
0cdf886
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nicely done!
I left a comment about how we surface the changed behaviour to users so that they are not caught off-guard by the missing default token.
@@ -633,3 +634,26 @@ func useAdmissionregistrationV1beta1(conn *kubernetes.Clientset) (bool, error) { | |||
useadmissionregistrationv1beta1 = ptrToBool(true) | |||
return true, nil | |||
} | |||
|
|||
func getServerVersion(connection *kubernetes.Clientset) (*gversion.Version, error) { | |||
sv, err := connection.ServerVersion() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We make use of this over here too:
serverVersion, err := conn.ServerVersion() |
Might be worth trying to unify the two.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, made a small improvement. 👍🏻
if err != nil { | ||
return diag.FromErr(err) | ||
} | ||
d.Set("default_secret_name", secret.Name) | ||
if b { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Silently skipping setting any value on default_secret_name
might be confusing to users with existing configurations.
I would suggest returning a Warning level Diagnostic when we do this, to alert users in the Terraform output about the new behaviour.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have updated the logic and moved the condition statement that verifies the version a bit lower. Now we should get a warning when creating or updating any of r/kubernetes_default_service_account
, d/kubernetes_service_account
, and r/kubernetes_service_account
, however, when we do the import for r/kubernetes_service_account
-- we don't get a warning message, because import expects only an error as return, and this is not an error.
0cdf886
to
646c065
Compare
… handle deprecated 'default_secret_name' in Kubernetes 1.24.0+
646c065
to
d732153
Compare
I applied this patch on top of terraform-provider-kubernetes, and am successfully able to create I however noted I always get the "Warning: 'default_secret_name' is no longer applicable for Kubernetes 'v1.24.0' and above" warning when creating a |
@alexsomesan hopefully this will be merged soon-ish™ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for fixing this @arybolovlev – I've approved but left a couple of cosmetic comments.
cee34c6
to
8caf26c
Compare
I would say this is a trade-off now. In the case of the managed Kubernetes cluster solution, this new behavior might not be delivered to the users and they might not be aware of it. So it can be a source of confusion when the provider does not produce desired output anymore. The same behavior you may observe with other tools in the Kubernetes world. They keep repeating the same warning or deprecation message a long time before it actually happens to make sure that everyone is aware of it. I hope that explanation makes sense. 😊 Thank you! |
I don't see how this explanation is related - the code is not using the |
…ccount' to handle deprecated 'default_secret_name' in Kubernetes 1.24.0+ (#1792) * Update resource and data of 'kubernetes_(default_)service_account' to handle deprecated 'default_secret_name' in Kubernetes 1.24.0+
I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. |
Description
Starting from the version
1.24.0
Kubernetes does not generate a token for a service account by default(link to the change log). In this case, the concept ofdefault_secret_name
is not applicable anymore for the Kubernetes clustersv1.24.0+
. This PR updates the provider's behavior accordingly to the cluster version.Affected resource
r/kubernetes_default_service_account
d/kubernetes_service_account
r/kubernetes_service_account
Acceptance tests
Output from acceptance testing:
1.24.0+
[1.24.2-gke.300]pre-1.24.0
[1.23.8-gke.400]Release Note
Release note for CHANGELOG:
References
Fixes: 1724
Community Note