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 new rancher setting patch in e2e #660

Merged
merged 1 commit into from
Aug 16, 2024

Conversation

alexander-demicev
Copy link
Member

What this PR does / why we need it:

For testing purposes we need to set agent-tls-mode to system-store. More details on the setting here: rancher/rancher#45684

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

Special notes for your reviewer:

Checklist:

  • squashed commits into logical changes
  • includes documentation
  • adds unit tests
  • adds or updates e2e tests

@alexander-demicev alexander-demicev requested a review from a team as a code owner August 14, 2024 14:56
@furkatgofurov7
Copy link
Contributor

@alexander-demicev I checked new setting was introduced in rancher-2.9? And we recently bumped it in turtles e2e #658. Should we re-trigger e2e tests once again to test it?

Signed-off-by: Alexandr Demicev <alexandr.demicev@suse.com>
@alexander-demicev
Copy link
Member Author

@furkatgofurov7 I rebased the PR

Copy link
Contributor

@furkatgofurov7 furkatgofurov7 left a comment

Choose a reason for hiding this comment

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

Deferring on CI, but this looks good

@Danil-Grigorev
Copy link
Contributor

Can you elaborate more why is this setting needed?

@furkatgofurov7
Copy link
Contributor

Can you elaborate more why is this setting needed?

@Danil-Grigorev rancher/rancher#45628 (comment) should give an idea

@Danil-Grigorev
Copy link
Contributor

Danil-Grigorev commented Aug 15, 2024

@furkatgofurov7 but this does not explain why we test this setting in our suite, does rancher missing this coverage? Since this has to be enabled with turtles on helm chart installation also to be effective for real import scenarios outside of e2e

@alexander-demicev
Copy link
Member Author

@Danil-Grigorev Defaulting to "strict" is a breaking change for anyone using ngrok in the way we're used to, when enabled allows only certificates verified with CAcerts authority provided by cacerts rancher setting will be used.
This is something that Rancher users have to configure on their own and shouldn't be related to how turtles import clusters. I believe that by using system-store we are not testing new functionality and using the setup we had before.
Here are a couple links to related PRs:
rancher/fleet#2507
rancher/rancher#45909

I'm not against extending the E2E suite to use "strict" mode for all our suites but it can done later separately.

@Danil-Grigorev
Copy link
Contributor

I'm confused @alexander-demicev as I was not advocating for using strict mode, see #660 (comment)

So if you saying that strict became new default, and by applying it we re-enables use of ngrok, change makes sense to me.

(Maybe this needs to be mentioned in PR description?)

@alexander-demicev
Copy link
Member Author

@Danil-Grigorev We will need to provide CA public key for ngrok(or anything else) if using strict, which shouldn't be a problem for us or users but it can be addressed separately

@alexander-demicev alexander-demicev merged commit 6a1a5be into rancher:main Aug 16, 2024
9 checks passed
@alexander-demicev alexander-demicev deleted the settingpatch branch August 30, 2024 10:10
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.

3 participants