-
Notifications
You must be signed in to change notification settings - Fork 115
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
ServiceAccount resources should be predeployed like ConfigMaps #221
Conversation
@@ -116,6 +116,7 @@ def create_or_update_secret(secret_name, secret_type, data) | |||
|
|||
secret_yaml = generate_secret_yaml(secret_name, secret_type, data) | |||
file = Tempfile.new(secret_name) | |||
|
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.
Spurious newline - let's aim for a minimal change set.
end | ||
# Expect deploying the unmanaged pod fails due to missing service account | ||
assert_deploy_failure(result) | ||
# if KUBE_CLIENT_VERSION < Gem::Version.new("1.8.0") |
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.
Is this a leftover from testing?
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.
Removed.
@@ -27,6 +27,51 @@ def test_full_hello_cloud_set_deploy_succeeds | |||
assert_logs_match(%r{ConfigMap/hello-cloud-configmap-data\s+Available}, 1) | |||
end | |||
|
|||
def test_service_account_predeployed_before_unmanaged_pod | |||
# Add an undefined service account in unmanaged pod | |||
service_account_name = "unknown-service-account" |
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.
Is the first part of this test adding much value?
The failure here comes from Kubernetes - we know the deployment will fail if it mentions a missing service account, so we're not testing k8s-deploy specific functionality. Unless I'm missing something, I'd suggest deleting this test case and keeping only the success case.
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 agree with @stefanmb on this.
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.
Agree. Removed the first part of the test which does not test feature provided kubernetes-deploy
@@ -27,6 +27,51 @@ def test_full_hello_cloud_set_deploy_succeeds | |||
assert_logs_match(%r{ConfigMap/hello-cloud-configmap-data\s+Available}, 1) | |||
end | |||
|
|||
def test_service_account_predeployed_before_unmanaged_pod | |||
# Add an undefined service account in unmanaged pod | |||
service_account_name = "unknown-service-account" |
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 agree with @stefanmb on this.
@@ -0,0 +1,7 @@ | |||
apiVersion: v1 |
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 don't think this new service account, and by extension your image pull secret, is ever being used, since your test uses the hello-cloud
fixture set, not the ejson-cloud
set. If you switch your test to use ejson-cloud, one option would be to make ejson-cloud's web.yaml always mount this new service account and have your test assert that it does so.
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.
Added another test "test_pod_get_image_pull_secret_from_service_account" in ejson-cloud to verify that if a pod does not contain any ImagePullSecrets, ImagePullSecrets of the pod's ServiceAccount should be added to the pod.
Test "test_service_account_predeployed_before_unmanaged_pod" in hello_cloud is to verify that service accounts are deployed before unmanaged pods.
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.
Isn't that basically just testing a kubernetes feature again? That's not something the gem is doing itself.
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.
Following our discuss, I have removed the test for image pull secret.
#179
Force deploy service accounts before unmanaged pods in redeployment.
Tested a service account is deployed before a pod that refers to it.
Tested that an image pull secret that a service account refers is deployed before all redeployed resources.