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.

Sign up for GitHub

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

Jump to bottom

Add legacyRemotingSecurityEnabled flag #625

Merged
merged 9 commits into from
May 18, 2022
Merged

Add legacyRemotingSecurityEnabled flag #625

merged 9 commits into from
May 18, 2022

Conversation

dominykas
Copy link
Contributor

@dominykas dominykas commented May 18, 2022 •
edited

What this PR does / why we need it

  • Implementation / reasoning described in Add a flag to explicitly disable remotingSecurity #616
  • Also updates whitespace in the unit tests - they were failing for me locally. Do they even get executed in CI? I don't see anything in the logs (even though the command is mentioned in ct.yaml)? It seems they're only failing for me locally... Is it due to helm version mismatch or smth? Seems CI is running 3.4, while I'm running 3.8...

Which issue this PR fixes

Special notes for your reviewer

Checklist

  • DCO signed
  • Chart Version bumped
  • CHANGELOG.md was updated

@dominykas dominykas requested a review from a team as a code owner May 18, 2022 13:34
timja
timja reviewed May 18, 2022
View reviewed changes
charts/jenkins/unittests/jcasc-config-test.yaml Outdated
url: https://RELEASE-NAME-jenkins:8080

- it: legacyRemotingSecurityEnabled = false (even though jenkins is 2.325 or older)
Copy link
Member

Choose a reason for hiding this comment

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

what happens if the tag is not a semver tag and legacyRemotingSecurityEnabled is not set?

I can't easily tell from reading the code.

(would be good if there was a test showing what it does)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

what happens if the tag is not a semver tag and legacyRemotingSecurityEnabled is not set?

It will error out.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, seems Helm does not have a nice way of handling this error or providing a nicer message... but adding that regex thing from #623 is also a bit overkill?

Copy link
Member

Choose a reason for hiding this comment

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

can we just back out the semver thing.

Default this legacy flag to false, document that people on x version or older will need to enable this flag if they want to keep it enabled

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I don't mind doing that, but that means a breaking change and a major version bump?

Copy link
Member

Choose a reason for hiding this comment

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

no issue with doing that. we should make changes as required

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, I think I got it right in the end. The signoff is killing me :) and the whitespace in the tests... Shall I open a PR to try to bump helm to latest?

Copy link
Member

Choose a reason for hiding this comment

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

yes that would be great

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cool, I'll open a separate PR once this one is merged, thanks

@dominykas
test: fix whitespace
3a523f1 
Signed-off-by: Dominykas Blyžė <[email protected]>
@timja
Copy link
Member

timja commented May 18, 2022

Is it due to helm version mismatch or smth? Seems CI is running 3.4, while I'm running 3.8...

Probably, at work we had some test failures updating to 3.8, something got stricter can't remember the specific issue

@dominykas dominykas changed the title test: make unit tests pass again Add legacyRemotingSecurityEnabled flag May 18, 2022
@dominykas
chore: remove legacy remotingSecurity behavior
5eac5af 
BREAKING CHANGES: removes the `remotingSecurity` flag based on `controller.tag`

Signed-off-by: Dominykas Blyžė <[email protected]>
@dominykas
chore: remove unused helper
06204f4 
Signed-off-by: Dominykas Blyžė <[email protected]>
timja
timja reviewed May 18, 2022
View reviewed changes
charts/jenkins/unittests/jcasc-config-test.yaml Outdated
@@ -1447,102 +1447,201 @@ tests:
location:
adminAddress:
url: https://RELEASE-NAME-jenkins:8080
- it: default config for jenkins 2.325 or older
- it: legacyRemotingSecurityEnabled = false (even though jenkins is 2.325 or older)
Copy link
Member

@timja timja May 18, 2022 •
edited

Choose a reason for hiding this comment

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

(probably don't commit from github due to signoff requirements...)

Suggested change
- it: legacyRemotingSecurityEnabled = false (even though jenkins is 2.325 or older)
- it: legacyRemotingSecurityEnabled = false

@dominykas
test: remove redunant info in test name
122e0bf 
Signed-off-by: Dominykas Blyžė <[email protected]>
timja
timja previously approved these changes May 18, 2022
View reviewed changes
timja
timja reviewed May 18, 2022
View reviewed changes
charts/jenkins/CHANGELOG.md Outdated
@@ -12,6 +12,10 @@ Use the following links to reference issues, PRs, and commits prior to v2.6.0.
The change log until v1.5.7 was auto-generated based on git commits.
Those entries include a reference to the git commit to be able to get more details.

## 4.0.0

Removes automatic `remotingSecurity` setting when using a container tag older than `2.236` (introduced in [`3.11.7`](#3117)). If you're using a version older than `2.236`, you should explicitly set `.controller.legacyRemotingSecurityEnabled` to `true`.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Removes automatic `remotingSecurity` setting when using a container tag older than `2.236` (introduced in [`3.11.7`](#3117)). If you're using a version older than `2.236`, you should explicitly set `.controller.legacyRemotingSecurityEnabled` to `true`.
Removes automatic `remotingSecurity` setting when using a container tag older than `2.326` (introduced in [`3.11.7`](#3117)). If you're using a version older than `2.326`, you should explicitly set `.controller.legacyRemotingSecurityEnabled` to `true`.

@dominykas
docs: fix typo
7e9ceb6 
Signed-off-by: Dominykas Blyžė <[email protected]>
@timja timja merged commit 55bfd28 into jenkinsci:main May 18, 2022
@dominykas dominykas deleted the issue-616 branch May 18, 2022 16:48
@dominykas dominykas mentioned this pull request May 18, 2022
Closed
@timja timja linked an issue May 18, 2022 that may be closed by this pull request
Possibility of semver mismatch #592
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@timja timja timja approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add a flag to explicitly disable remotingSecurity Possibility of semver mismatch
2 participants
@dominykas @timja