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

Authentikos: Refactor Token/Secret creators to interfaces. #3036

Closed
wants to merge 7 commits into from

Conversation

wlynch
Copy link

@wlynch wlynch commented Nov 2, 2020

This allows for other token types to be created while reusing the same
common creation / reconcilation components.

As an example for how this can be extended, adds a GitHub App token
creator as an alternative to the existing Google OAuth implementation.

Also changes secret updates to patches of just the data to fix an issue
dicovered during testing where the reconciler would overwrite any
user-added labels/annotations.

Implements #2659

@wlynch wlynch requested review from a team as code owners November 2, 2020 17:06
@istio-policy-bot
Copy link

😊 Welcome @wlynch! This is either your first contribution to the Istio test-infra repo, or it's been
awhile since you've been here.

You can learn more about the Istio working groups, code of conduct, and contributing guidelines
by referring to Contributing to Istio.

Thanks for contributing!

Courtesy of your friendly welcome wagon.

@google-cla google-cla bot added the cla: yes Set by the Google CLA bot to indicate the author of a PR has signed the Google CLA. label Nov 2, 2020
@istio-testing istio-testing added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Nov 2, 2020
@istio-testing
Copy link
Collaborator

Hi @wlynch. Thanks for your PR.

I'm waiting for a istio member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@wlynch wlynch force-pushed the github-app-authentikos branch 2 times, most recently from 43f257a to 9e62172 Compare November 2, 2020 18:06
@istio-testing istio-testing added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Nov 2, 2020
@@ -2,944 +2,58 @@ load("@bazel_gazelle//:deps.bzl", "go_repository")

def go_repositories():
Copy link
Author

Choose a reason for hiding this comment

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

Bit of a large diff here, though looks like a lot of reorderings - this was generated via gazelle update-repos -from_file=go.mod -to_macro=repos.bzl%go_repositories from the authentikos directory. Let me know if there's a better way to update this file!

@ericvn
Copy link
Contributor

ericvn commented Nov 2, 2020

/ok-to-test

@istio-testing istio-testing added ok-to-test Set this label allow normal testing to take place for a PR not submitted by an Istio org member. and removed needs-ok-to-test labels Nov 2, 2020
This allows for other token types to be created while reusing the same
common creation / reconcilation components.

As an example for how this can be extended, adds a GitHub App token
creator as an alternative to the existing Google OAuth implementation.

Also changes secret updates to patches of just the data to fix an issue
dicovered during testing where the reconciler would overwrite any
user-added labels/annotations.
@wlynch wlynch force-pushed the github-app-authentikos branch from 248249b to 1629aa3 Compare November 4, 2020 17:59
path.

The gitignore was configured to ignore the authentikos binary generated
by `go build`, but because of the gitignore syntax this resulted in git
ignoring any directory with "authentikos" in the path.

Also adds in missing files that were previously being ignored.
@wlynch wlynch force-pushed the github-app-authentikos branch from 1629aa3 to a8a6fd7 Compare November 4, 2020 18:34
If running as root (i.e. default user for docker run), setting the file
permissions to `0000` doesn't actually prevent the file from being
unreadable. This was causing false signals in CI testing. This modifies
the behavior to just create a file that doesn't exist, which has the
same effect of what we're trying to test (e.g. that we can't read the
template file).

Since the deletedCredsFile is similar in this sense this was also
changed to be consistent.

Also added some minor test QoL changes (usage of t.Helper,
t.Log) to aid future debugging.
Adds missing support for mixing standard golang and pflag flags. See
https://github.com/spf13/pflag#supporting-go-flags-when-using-pflag for
details.
This includes fixes for a variety of issues related to integration
testing:

- Use `kind load docker-image` to pass in the local image to the test.
  - Disable remote pulling of authentikos images to prevent pulling a
    non-test image
  - Modify integ-test makefile rule to depend on fresh image builds.
- Update `kindest/node` version to latest v1.17.x image. This is
  required to support host networking DNS resolution that is broken in
  v1.17.0.
- Modify unit tests to be more OSX friendly (`xargs -r` doesn't exist on
  OSX)
@istio-testing istio-testing added the needs-rebase Indicates a PR needs to be rebased before being merged label Jun 6, 2021
@istio-testing
Copy link
Collaborator

@wlynch: PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@istio-testing
Copy link
Collaborator

@wlynch: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Rerun command
pull-test-infra-check-testgrid-config-transfigure 48fde29 link /test pull-test-infra-check-testgrid-config-transfigure

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@wlynch wlynch closed this Sep 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Set by the Google CLA bot to indicate the author of a PR has signed the Google CLA. needs-rebase Indicates a PR needs to be rebased before being merged ok-to-test Set this label allow normal testing to take place for a PR not submitted by an Istio org member. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants