Skip to content
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

Add secrets filter #72

Merged
merged 5 commits into from Mar 13, 2019

Conversation

3 participants
@martindekov
Copy link
Member

martindekov commented Feb 16, 2019

Adding filter which filters which secrets
we create, since ATM we create all secrets
when we dont need big part of them

Signed-off-by: Martin Dekov mdekov@vmware.com

Description

Same as this PR.

TL:DR: Adding secrets filter.

Closes #59

How Has This Been Tested?

Manually. Check reference above.

Checklist:

I have:

  • checked my changes follow the style of the existing code / OpenFaaS repos
  • updated the documentation and/or roadmap in README.md
  • read the CONTRIBUTION guide
  • signed-off my commits with git commit -s
  • added unit tests
@doowb

This comment has been minimized.

Copy link
Contributor

doowb commented Feb 16, 2019

@martindekov I'm testing out deploying, so I'll test out this PR and let you know if I run into any issues.

@martindekov

This comment has been minimized.

Copy link
Member Author

martindekov commented Feb 17, 2019

Hey @doowb thanks for that! I tested it in isolation, also you might apply your own filters and features ATM thats how the logic is done you can check the code

Show resolved Hide resolved example.init.yaml Outdated
@alexellis

This comment has been minimized.

Copy link
Member

alexellis commented Mar 2, 2019

@martindekov are you still working on this? If so please give an update when you can and provide a rough estimate. 👍 This is something we need to get in and fixed up before we can proceed with the GitLab work / merge.

@alexellis alexellis changed the title Add secrets filter WIP Add secrets filter Mar 2, 2019

@martindekov

This comment has been minimized.

Copy link
Member Author

martindekov commented Mar 3, 2019

Hey Alex I will do my best to update the PR by EOD tomorrow with your suggestions.

Show resolved Hide resolved example.init.yaml Outdated
Show resolved Hide resolved example.init.yaml Outdated
Show resolved Hide resolved example.init.yaml Outdated
@alexellis

This comment has been minimized.

Copy link
Member

alexellis commented Mar 4, 2019

Thanks for working on this Martin.

I think it's close, but we have a couple of things to adjust:

  • I can see some conflicts on the files (rebase needed)
  • s3 / internal_trust are mapping to features, but I don't think we need it because it is mandatory and can't be turned off. What do we gain by adding the filter? It seems confusing.

Alex

@martindekov martindekov force-pushed the martindekov:martindekov/59_add_secrets_filter branch from 42b3fc2 to 0fe83b9 Mar 4, 2019

@martindekov martindekov changed the title WIP Add secrets filter Add secrets filter Mar 4, 2019

@martindekov

This comment has been minimized.

Copy link
Member Author

martindekov commented Mar 4, 2019

I had some problems due to this PR being inactive and the commit fell behind so I cherry-pick the commits and referenced the comments. I think this is ready now 👍

Thanks for the quick review @alexellis you can check it out again when you can

@martindekov

This comment has been minimized.

Copy link
Member Author

martindekov commented Mar 4, 2019

Also if everything is fine I will squash

@martindekov martindekov force-pushed the martindekov:martindekov/59_add_secrets_filter branch from 0fe83b9 to 64a64c8 Mar 4, 2019

Show resolved Hide resolved main.go
Show resolved Hide resolved main.go Outdated
Show resolved Hide resolved main.go Outdated
Show resolved Hide resolved main.go Outdated
Show resolved Hide resolved main.go Outdated
Show resolved Hide resolved main.go Outdated
literals:
- name: s3-secret-key
filters:
- "default"

This comment has been minimized.

@alexellis

alexellis Mar 5, 2019

Member

Do we need an explicit filter of default when these are non-optional? (Tell me a bit about the design/approach)

This comment has been minimized.

@martindekov

martindekov Mar 5, 2019

Author Member

If you look the code this plan = filterFeatures(plan) is the place I check the struct and append the features according to what is populated. I always add default filter to the features so when filtering secrets, the mandatory ones are always created. That way the filtering of secrets is easier with 1 function which just checks if the filters field of the secret contains one of the available features which are enabled/configured in the .yaml. In case you want secret created, for whatever reason, create your secret and add default filter and it will be created. I will document that if necessary.

Show resolved Hide resolved main.go Outdated

@martindekov martindekov force-pushed the martindekov:martindekov/59_add_secrets_filter branch from df94f50 to 5673b89 Mar 6, 2019

@alexellis

This comment has been minimized.

Copy link
Member

alexellis commented Mar 7, 2019

There's a small rebase needed on pkg/types/types.go. After resolving that let me know if this is still working e2e with TLS + Auth and I'll merge.

Thank you for working on this 👍

Alex

@martindekov martindekov force-pushed the martindekov:martindekov/59_add_secrets_filter branch from 130ad96 to 1f049e4 Mar 7, 2019

@martindekov

This comment has been minimized.

Copy link
Member Author

martindekov commented Mar 7, 2019

There are two registry-secret in example.init.yml. Is this intended?

  - name: "registry-secret"
    files:
      - name: "config.json"
        value_from: "~/.docker/config.json"
    filters:
      - "default"
    namespace: "openfaas"
  - name: "registry-secret"
    files:
      - name: ".dockerconfigjson"
        value_from: "~/.docker/config.json"
    namespace: "openfaas-fn"
    type: "kubernetes.io/dockerconfigjson"

martindekov added some commits Feb 9, 2019

Add secrets filter
Adding filter which filters which secrets
we create, since ATM we create all secrets
when we dont need big part of them

Signed-off-by: Martin Dekov <mdekov@vmware.com>
Adding dynamic loading of features
Add dynamic loading of features as the
previous implementation forced the user
to add the features in the yaml file in
effect doing it twice

Signed-off-by: mdekov <mdekov@vmware.com>
Remove s3 and internal_trust config
Removing s3 and internal_trust config
as they are mandatory replacing with
default feature which is always turned
on. Also fixing conflicts and removing
debug code along with formatting
comments in the example yaml

Signed-off-by: mdekov <mdekov@vmware.com>
Remove basic auth config and add unit tests
Removing basic auth configuration and the
creation of credentials is now handled
by the default filter. Also adding unit
tests to showcase what are the changes

Signed-off-by: Martin Dekov <mdekov@vmware.com>
Add filesExist function
Adding filesExist function which copies the
block of code which checks if the secret
files exist

Signed-off-by: Martin Dekov <mdekov@vmware.com>

@martindekov martindekov force-pushed the martindekov:martindekov/59_add_secrets_filter branch from 1f049e4 to ddd683f Mar 7, 2019

@martindekov

This comment has been minimized.

Copy link
Member Author

martindekov commented Mar 7, 2019

Hey @alexellis appended the default to the new kubernetes secret. Maybe when GitLab and Swarm integrations are merged I will open New PR to add filtering for those two. Otherwise the secrets creation manual tests are tested in this gist:
https://gist.github.com/martindekov/05ee650d74e283cad953b5bedec8f33c 👍

@alexellis

This comment has been minimized.

Copy link
Member

alexellis commented Mar 8, 2019

You should see registry-secret and registry-pull-secret now. Let me know if this is ready for merging.

@alexellis

This comment has been minimized.

Copy link
Member

alexellis commented Mar 8, 2019

In the gist I saw:

Plan completed in 1.821173 seconds

That can't be right?

We'll also want to do a regression test to see that the DNS is being picked up when it should be?

@martindekov

This comment has been minimized.

Copy link
Member Author

martindekov commented Mar 11, 2019

Ok missed the comments, sorry about that I will do my best to test it E2E by EOD. And I will rebase with the latest secrets I will still append the default filter to those two since support for swarm is not yet added I see pending PR.

@martindekov

This comment has been minimized.

Copy link
Member Author

martindekov commented Mar 11, 2019

In the gist I saw:
Plan completed in 1.821173 seconds
That can't be right?

The gist shows the secret creation only if I populate values in the yaml. The unit tests also covers this part it is not E2E in the sense it deploys the cloud from scratch.

@alexellis alexellis merged commit 6583818 into openfaas-incubator:master Mar 13, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@alexellis

This comment has been minimized.

Copy link
Member

alexellis commented Mar 13, 2019

Thanks @martindekov I'll merge 👍

@alexellis alexellis referenced this pull request Mar 13, 2019

Merged

Add GitLab integration #46

5 of 5 tasks complete
@martindekov

This comment has been minimized.

Copy link
Member Author

martindekov commented Mar 13, 2019

My pleasure Alex 🍻

@martindekov martindekov moved this from Review to Merge in Triage - Code/Review/Merge Mar 15, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.