-
Notifications
You must be signed in to change notification settings - Fork 221
Custom auth fixes, tests, and CI framework #422
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
Merged
Merged
Changes from 15 commits
Commits
Show all changes
16 commits
Select commit
Hold shift + click to select a range
85bc33d
Custom auth fixes and CI framework attempt #2
bretambrose 6c8ec30
Running tests is cool
bretambrose 0b46dbe
Testing
bretambrose 07d3462
Use new unit test role for running unit tests
4f7ee68
Just linux for now
412a141
Try to use DA creds on linux
2a963e7
Testing
94f5e36
Closer
bf27fe6
Closer
f8f33fa
Experiment with web identity assume role
bretambrose 7cc75ce
Non-internal-container builds working, move to CRT containers (al2, r…
bretambrose 6689da9
Revert all build creds to CRT container role regardless of needing co…
bretambrose 3b184c0
Remove CRT container jobs
bretambrose c2a225d
Direct custom auth tests and configuration
bretambrose 56c008e
Websocket custom auth tests and bug fixes
bretambrose 9a3a921
Merge branch 'main' into CustomAuthTake2
bretambrose File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,29 @@ | ||
import Builder | ||
import re | ||
|
||
class CiTest(Builder.Action): | ||
|
||
def _write_environment_script_secret_to_env(self, env, secret_name): | ||
mqtt5_ci_environment_script = env.shell.get_secret(secret_name) | ||
env_line = re.compile('^export\s+(\w+)=(.+)') | ||
|
||
lines = mqtt5_ci_environment_script.splitlines() | ||
for line in lines: | ||
env_pair_match = env_line.match(line) | ||
if env_pair_match.group(1) and env_pair_match.group(2): | ||
env.shell.setenv(env_pair_match.group(1), env_pair_match.group(2), quiet=True) | ||
|
||
|
||
def run(self, env): | ||
|
||
actions = [] | ||
|
||
try: | ||
self._write_environment_script_secret_to_env(env, "ci/sdk-unit-testing") | ||
|
||
env.shell.exec(["python3", "-m", "unittest", "discover", "--verbose"], check=True) | ||
except: | ||
print(f'Failure while running tests') | ||
actions.append("exit 1") | ||
|
||
return Builder.Script(actions, name='ci-test') |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.
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.
Minor: I'd prefer if this was hidden in a GitHub secret. Technically there is no reason to put it in a secret, as it cannot be used without authenticating the IAM role on a per-repo basis, but I feel some additional security through obscurity couldn't hurt. It also makes it easier to change the IAM role in the future without needing to make a commit/PR.
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.
Well I have the opposite conviction. I'm convinced we need to remove all Github secrets for maintainability reasons (do you know what IAM policy is bound to the credentials embedded in some of the repos? I don't and I have no way of finding out).
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.
Hmm, fair point. That is something I had not considered and I can see how having it hidden away in secrets would make it impossible to know exactly what credentials are bound to what IAM role. Over time, I could see this being an issue, especially as it is forgotten why/when/who setup what.
Hiding it in secrets provides only a tiny bit of extra security, so, there's that too. It does expose the AWS account ID, but I don't think there is any harm in that? I cannot think of any right off at least. Since the IAM roles cannot be used outside our repositories, I do not think there is any worries there either.
In that case though, should we change this for all the IAM role secrets in all 4 of the V2 IoT SDK repositories? I think we should be consistent with how we do it, and so if we want to not use GitHub secrets for IAM roles, then we should apply this change across the board in our repositories.
Obviously this is outside the scope of this particular PR, but if we want to take this approach, then I can make PRs to do this as I know what IAM roles go to what secrets.
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 think we should aim for no secrets and yes that requires quite a bit more work. I also think we should move tests away from requiring the test fixture to query secrets and instead make everything environmental injection via script from builder actions. That gives us a pattern that is the same across all repos, regardless of language, and the amount of resources and set up is significantly lessened.
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.
Okay. As a first step, I'll work on removing the IAM role secrets first 👍