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

Adding volumes, configmaps, secrets and certificates and restructuring the code again #128

Merged
merged 9 commits into from Aug 26, 2020

Conversation

aliciafmachado
Copy link
Collaborator

@aliciafmachado aliciafmachado commented Aug 25, 2020

Now, all the expected functionalities work: volumes, pow, deployment, service, ingress, certificate, secrets, etc.
About the secrets, I decided to create them in the kube-system and then copy them to other namespaces.
I added as well two new fields in the CRD: domainName and replicas.
Also, made some changes in the structure of the code, by passing the update functions to each package, so we have always a set of functions for the packages, unless one/some of them is/are unnecessary: create, delete, generate, isEqual and Update.
But it's still necessary to: finish the implementation of the status and maybe add others status; change the scripts; and finally add documentation.

…ows more meaningful informations about the objects.
…st path of the persistent volumes. Changed status fields as well but didn't implement them yet.
…nction in the challenge_controller.go. Also, fixed a problem with pow and create a package for status.
…m namespace. Also, implemented ManagedCertificate.
…allenge namespace. Also, fixed a condition for creating the certificate.
…rator. Also, created a samples folder for CR samples. Updated operator.yaml and uncommented the rest of the configurations in the deployment.
Copy link
Collaborator

@gree-gorey gree-gorey left a comment

Choose a reason for hiding this comment

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

Overall LGTM, see my comments.

I think that using ConfigMap for powDifficulty is a bit of over-engineering but I understand that this is not smth you developed, you just migrated it from the Makefile. I think the problem is as simple as "propagating an integer from config into a container" and should be solved by adding the ENV variable into Pod. That would require though to change pow script to read the ENV instead of the file here: https://github.com/google/kctf/blob/alpha/base/nsjail-docker/files/proof_of_work/maybe_pow.sh#L21.

But that is probably better for separate Issue/PR. This one is working and that's okay 👍

kctf-operator/pkg/apis/kctf/v1alpha1/challenge_types.go Outdated Show resolved Hide resolved
kctf-operator/pkg/apis/kctf/v1alpha1/challenge_types.go Outdated Show resolved Hide resolved
@sirdarckcat
Copy link
Member

Overall LGTM, see my comments.

I think that using ConfigMap for powDifficulty is a bit of over-engineering but I understand that this is not smth you developed, you just migrated it from the Makefile. I think the problem is as simple as "propagating an integer from config into a container" and should be solved by adding the ENV variable into Pod. That would require though to change pow script to read the ENV instead of the file here: https://github.com/google/kctf/blob/alpha/base/nsjail-docker/files/proof_of_work/maybe_pow.sh#L21.

But that is probably better for separate Issue/PR. This one is working and that's okay 👍

I think the only downside of that is that we need to restart the deployment for it to take effect?

@aliciafmachado aliciafmachado merged commit 74961e3 into beta Aug 26, 2020
@aliciafmachado aliciafmachado deleted the volumes-and-pow branch August 26, 2020 16:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants