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

Feature/fix maven and redis and okta checks #456

Open
wants to merge 6 commits into
base: develop
Choose a base branch
from

Conversation

duckhead
Copy link

Here's the little nits I've found over the past few days. Nothing incredible earth shattering, but nice to haves.

… This fix is slightly incomplete, the data is still in Redis, it just will stop querying for more. Current. the existing data cannot be deleted.
@duckhead
Copy link
Author

I assume it's normal for the tests to fail. Runs fine from where I sit, which is on a Debian bookworm box.

@JohnKallies
Copy link
Contributor

I assume it's normal for the tests to fail. Runs fine from where I sit, which is on a Debian bookworm box.

It's not normal to fail. The issue reported seems to be around the grep for the API test example file as well. Let's look closer at the GH Actions log? I'm looking as well.

@JohnKallies
Copy link
Contributor

I assume it's normal for the tests to fail. Runs fine from where I sit, which is on a Debian bookworm box.

Try adding this at line 81 in osmt_cli.sh

echo_info "Okta values are not provided by environment variables. Sourcing ${env_file} env file."

@duckhead
Copy link
Author

I assume it's normal for the tests to fail. Runs fine from where I sit, which is on a Debian bookworm box.

It's not normal to fail. The issue reported seems to be around the grep for the API test example file as well. Let's look closer at the GH Actions log? I'm looking as well.
That won't wor

I assume it's normal for the tests to fail. Runs fine from where I sit, which is on a Debian bookworm box.

Try adding this at line 81 in osmt_cli.sh

echo_info "Okta values are not provided by environment variables. Sourcing ${env_file} env file."

That won't work. First, I think it's common.sh you want me to change. This change will basically bypass reading the editable file (osmt_apitest.env) if we supplied env variables. Since the test runner isn't supplying those, we will attempt to source it. That file doesn't exist, because as far as I know, osmt_cli.sh -i hasn't run. Are you saying we should load the osmt_apitest.env.example file it the osmt_apitest.env file doesn't exist?

@JohnKallies
Copy link
Contributor

That won't work. First, I think it's common.sh you want me to change.

You are right, it is the library file

This change will basically bypass reading the editable file (osmt_apitest.env) if we supplied env variables. Since the test runner isn't supplying those, we will attempt to source it. That file doesn't exist, because as far as I know, osmt_cli.sh -i hasn't run. Are you saying we should load the osmt_apitest.env.example file it the osmt_apitest.env file doesn't exist?

No, these values should be provided by the runner. It looks like they aren't , so I was looking for log output that would confirm that.

@duckhead
Copy link
Author

That won't work. First, I think it's common.sh you want me to change.

You are right, it is the library file

This change will basically bypass reading the editable file (osmt_apitest.env) if we supplied env variables. Since the test runner isn't supplying those, we will attempt to source it. That file doesn't exist, because as far as I know, osmt_cli.sh -i hasn't run. Are you saying we should load the osmt_apitest.env.example file it the osmt_apitest.env file doesn't exist?

No, these values should be provided by the runner. It looks like they aren't , so I was looking for log output that would confirm that.

OK, reverting test/osmt-apitest.env.example and rerunning osmt_cli.sh -i fixes the issue. So, it is my change. The mvn verify... makes it much further with the file reverted.

I don't know much more yet, I've never tried tracing into a maven command before. But, i suspect that we are failing because we are now seeing XXXXXX. So, some kind of validation is failing, regardless of specified environment variables or command lines. The solution should be to not do that validation if we have the inputs otherwise, just like we won't source the .env file we have the inputs otherwise. I don't know where that is yet though.

For my own edification, why use username/password here, and the OpenID stuff in api/ is that just some work that wasn't finished?

@duckhead
Copy link
Author

duckhead commented Oct 2, 2023

OK, I have no idea how to trace down what Maven is doing. But, here's my suspicion. At some point, we perform the same checks that osmt_cli.sh -v does. We see that we have xxx in the .env file, and exit out.

Before, the values were XXX and always passed. Now they are not, and always fail. The fix, from where I set, would be to do the same check that is done in that code you pointed me to earlier (line 81 in common.sh, the 3 -n checks) and add that into _validate_env_fille. Or, maybe at the caller instead, where we knew we are running the API tests.

I'll try and play with that tomorrow and see what I get, If that fixes it, cool. If not, I'm going to need to know what code checks the config and decides it's not worthy.

@JohnKallies
Copy link
Contributor

@duckhead it's failing before any validation, on the test for "file exists" and "can read the file". I double checked that the secrets OKTA_ still exist in the repo, they are good.

I added a PR for the additional logging I mentioned, and added you as a reviewer. Would you please give it a thumb up?

@JohnKallies
Copy link
Contributor

JohnKallies commented Oct 2, 2023

Maven is simply calling the pre_test_setup.sh script, which sources the common functions, declares some of its own functions, and then calls main. The first thing it does is source osmt_apitest.env, if the 3 OKTA_ vars don't exist. I checked them, and they are still being provided to the runner. I really don't see how your change could impact that. Still looking at it...

@duckhead
Copy link
Author

duckhead commented Oct 2, 2023

Maven is simply calling the pre_test_setup.sh script, which sources the common functions, declares some of its own functions, and then calls main. The first thing it does is source osmt_apitest.env, if the 3 OKTA_ vars don't exist. I checked them, and they are still being provided to the runner. I really don't see how your change could impact that. Still looking at it...

I do understand what's going on, and I think my analysis above is correct. I'm just not expressing it well. I think it's a little easier with your one-liner though, so I'll try again. This is what I'm getting after I applied the one line change:

INFO: OAUTH values are provided by environment variables. Not sourcing /home/jfrank/osmt/api/osmt-dev-stack.env env file.

INFO: Checking /home/jfrank/osmt/api/osmt-dev-stack.env for invalid default values (xxxxxx)
INFO: No invalid default values in /home/jfrank/osmt/api/osmt-dev-stack.env
INFO: Sourcing /home/jfrank/osmt/api/osmt-dev-stack.env

So, your new one liner is called first, and it did the right thing. It found we had the env variables, and didn't source the file. The next line is only found in common.sh, in _validate_env_file. So, we are calling that. That is only called from osmt_cli.sh -v, in this line:

/osmt_cli.sh:42: _validate_env_file "${APITEST_ENV_FILE}" || is_environment_valid+=1

The issue is that we need to do a -n on the 3 OKTA variables here, and if those are set, skip this line, but still add one to is_environment_valid. Does that make any more sense?

@JohnKallies
Copy link
Contributor

Unless I'm missing it, there shouldn't be any env file validation or dependency in a GH Action workflow. They are not created, because the secrets are provided by the runner.

@duckhead
Copy link
Author

duckhead commented Oct 2, 2023

I agree, there should be no file validation, but the log sure seems to say there is. So, that leaves a couple possibilities:

  1. It's not validating the files, and I can't read logs.
  2. It should not be validating files, but it is. And that needs to be fixed.

If it's #1, I have no idea what to do next.
If it's #2, I know a fix (fix the validation code to skip validation if we have the env variables), but it sounds like you're favoring a different fix (don't validate anything (or maybe just not the files)) In that case, maybe add a --skip-file-validation to osmt_cli.sh seems like the easiest fix.

So, are we talking #1 or #2?

@duckhead
Copy link
Author

duckhead commented Oct 3, 2023

At great cost to my sanity, here's how to see what the issue is:

`--- a/test/bin/pre_test_setup.sh
+++ b/test/bin/pre_test_setup.sh
@@ -37,6 +37,7 @@ error_handler() {

main() {

Sourcing API test env file

  • echo ${OKTA_URL} ${OKTA_USERNAME} ${OKTA_PASSWORD}
    source_env_file_unless_provided_okta "${PROJECT_DIR}/test/osmt-apitest.env"
    source_env_file "${PROJECT_DIR}/test/bin/osmt-apitest.rc"`

I have no idea why that's not accepting the code markdown. I give up...

If you add that and run the test, you will find that these variables are not set. I don't know what sets those, so I'll have to leave that up to you guys.

@JohnKallies
Copy link
Contributor

@duckhead please rebase from develop to pick up #457. If we still a problem with your changes, let's back out the XXX lower case fix and get the benefit of your other 2 changes?

@duckhead
Copy link
Author

duckhead commented Oct 3, 2023

@duckhead please rebase from develop to pick up #457. If we still a problem with your changes, let's back out the XXX lower case fix and get the benefit of your other 2 changes?

OK, I've rebased and rolled back the XXX change. I also put in the docker version fix, and the echo_info I used to find that the variables were missing, but that is commented out for now, since it still needs addressing.

@JohnKallies
Copy link
Contributor

I don't see where you've rebased my changes from develop in yet.
image

I don't think my changes will save the day, but I'm trying to run down how changes from a committer vs a maintainer affects how GH Runners consume secrets. I'd like to see those log entries in context in your build output. Would you please rebase from develop when you get a moment? Thank you

@duckhead
Copy link
Author

duckhead commented Oct 4, 2023

I don't see where you've rebased my changes from develop in yet. image

I don't think my changes will save the day, but I'm trying to run down how changes from a committer vs a maintainer affects how GH Runners consume secrets. I'd like to see those log entries in context in your build output. Would you please rebase from develop when you get a moment? Thank you

I'm a command-line guy, that way I can easily keep track of what I've messed up. I did this today, but I did the same yesterday.

jfrank@yellow:/osmt$ git rebase --onto develop
Successfully rebased and updated refs/heads/feature/fixMavenAndRedisAndOktaChecks.
frank@yellow:
/osmt$ git pull
Updating c3a80f9..900ad76
Fast-forward
bin/lib/common.sh | 2 --
docker/base-images/Dockerfile.osmt-build | 2 +-
docker/dev-stack.yml | 1 +
test/bin/pre_test_setup.sh | 1 +
test/osmt-apitest.env.example | 4 ++--
5 files changed, 5 insertions(+), 5 deletions(-)
jfrank@yellow:~/osmt$ git push
Everything up-to-date

@duckhead
Copy link
Author

duckhead commented Oct 4, 2023

This is the line that's missing, right, because I see it in my output. So it's there, no matter what git says...

INFO: OAUTH values are provided by environment variables. Not sourcing /home/jfrank/osmt/api/osmt-dev-stack.env env

@JohnKallies
Copy link
Contributor

Hang on.... I've been looking at your log output in the GH Actions. Are you having these failures locally? You should not be having issues locally if you have created your local osmt-apitest.env file and provided the proper Okta values there.

Please clarify?

@duckhead
Copy link
Author

duckhead commented Oct 4, 2023

Hang on.... I've been looking at your log output in the GH Actions. Are you having these failures locally? You should not be having issues locally if you have created your local osmt-apitest.env file and provided the proper Okta values there.

Please clarify?

I am having these issues locally. It's because unlike in the API tests, you want username/password here. And, I don't have, nor can I find, a username/password anywhere in OKTA. That was why I asked why we did it this way above somewhere.

@JohnKallies
Copy link
Contributor

The intention here is to add your personal Okta user ID / password / auth endpoint in the env file.
image

Would you please confirm that you have this locally and how your API test Maven profile execution completes?

Also, let's discuss more directly? Email me at [email protected] if you are willing? I don't (can't) do this for community support, but we are starting to talk about auth issues and it might be easier / safer off GitHub.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants