Skip to content
This repository has been archived by the owner on Dec 16, 2020. It is now read-only.

Update secret handler for PUT method #66

Merged
merged 1 commit into from
Jan 7, 2020

Conversation

viveksyngh
Copy link
Contributor

@viveksyngh viveksyngh commented Jan 5, 2020

This commit updates secrets handler for PUT method. It returns 405
"Method Not Allowed" for PUT method becuase secrets in docker swarm are
immutable.

Fixes: #65

Signed-off-by: Vivek Singh [email protected]

Description

Motivation and Context

  • I have raised an issue to propose this change (required)

#65

How Has This Been Tested?

I have tested this on Docker For Mac with Docker Swarm. For testing use viveksyngh/faas-swarm:latest-dev docker image.

faas-cli secret ls
WARNING! Communication is not secure, please consider using HTTPS. Letsencrypt.org offers free SSL/TLS certificates.
No secrets found.
➜  faas-swarm git:(issue-65) ✗ faas-cli secret create test-secret --from-literal=test-value
WARNING! Communication is not secure, please consider using HTTPS. Letsencrypt.org offers free SSL/TLS certificates.
Creating secret: test-secret
Created: 201 Created
➜  faas-swarm git:(issue-65) ✗ faas-cli secret ls
WARNING! Communication is not secure, please consider using HTTPS. Letsencrypt.org offers free SSL/TLS certificates.

NAME
test-secret

➜  faas-swarm git:(issue-65) ✗ faas-cli secret update test-secret --from-literal=update-value
WARNING! Communication is not secure, please consider using HTTPS. Letsencrypt.org offers free SSL/TLS certificates.
Updating secret: test-secret
server returned unexpected status code: 405 - %                                                                                                                                   ➜  faas-swarm git:(issue-65) ✗ faas-cli secret rm test-secret
WARNING! Communication is not secure, please consider using HTTPS. Letsencrypt.org offers free SSL/TLS certificates.
Removed.. OK.
➜  faas-swarm git:(issue-65) ✗ faas-cli secret ls
WARNING! Communication is not secure, please consider using HTTPS. Letsencrypt.org offers free SSL/TLS certificates.
No secrets found.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I've read the CONTRIBUTION guide
  • I have signed-off my commits with git commit -s
  • I have added tests to cover my changes.
  • All new and existing tests passed.

}
}

return nil, fmt.Errorf("not found secret with name: %s", name), http.StatusNotFound
return nil, http.StatusNotFound, fmt.Errorf("not found secret with name: %s", name)
Copy link
Member

Choose a reason for hiding this comment

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

unable to find secret with name: %s

@@ -51,7 +51,8 @@ func MakeSecretsHandler(c client.SecretAPIClient) http.HandlerFunc {
responseStatus, responseBody, responseErr = createNewSecret(c, body)
break
case http.MethodPut:
responseStatus, responseBody, responseErr = updateSecret(c, body)
responseStatus = http.StatusMethodNotAllowed
responseErr = fmt.Errorf("docker swarm secrets are immutable")
Copy link
Member

Choose a reason for hiding this comment

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

faas-swarm is unable to update secrets, delete and re-create or use a new name

Copy link
Member

Choose a reason for hiding this comment

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

Please could you add a unit test for this scenario if we don't have one yet?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We already had unit test for this scenario, I have updated it.

Copy link
Member

@alexellis alexellis 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, I'm requesting a couple of changes. Please ping me when done or if you have Qs.

This commit updates secrets handler for PUT method. It returns 405
"Method Not Allowed" for PUT method becuase secrets in docker swarm are
immutable.

Fixes: openfaas#65

Signed-off-by: Vivek Singh <[email protected]>
Copy link
Member

@alexellis alexellis left a comment

Choose a reason for hiding this comment

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

LGTM

@alexellis alexellis merged commit 48de05f into openfaas:master Jan 7, 2020
@alexellis alexellis mentioned this pull request Jan 7, 2020
11 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

UpdateSecret function in secrets.go handler will not work as Docker secrets are immutable
2 participants