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

[WFLY-15452] - allow specifying AJP_ALLOWED_REQUEST_ATTRIBUTES_PATTERN #14789

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

baranowb
Copy link
Contributor

@baranowb baranowb commented Oct 13, 2021

@github-actions github-actions bot added the deps-ok Dependencies have been checked, and there are no significant changes label Oct 13, 2021
kabir
kabir previously requested changes Oct 25, 2021
Copy link
Contributor

@kabir kabir left a comment

Choose a reason for hiding this comment

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

@baranowb I think you need a transformers tests (apologies if there is one and I missed it).

Also, you need to bump UndertowExtension.CURRENT_MODEL_VERSION, since 11.0.0 has already been in a release and you're changing the model

@baranowb
Copy link
Contributor Author

@kabir Thought bump to 12.1 should be enough with just addition of attribute? There is test for model version( subsystem).
ATM Im waiting for @fl4via to clarify some transformer versions vs tests definition, as they seem to be misaligned.

@kabir
Copy link
Contributor

kabir commented Oct 26, 2021

@baranowb Yes, bumping the xml was good - you also need to bump the management model version.

@baranowb
Copy link
Contributor Author

baranowb commented Oct 27, 2021

@kabir Uh, missed that, but it seems to be a fallout from previous bump, as current was 12, not 11.
I wonder why it did not fail.

Copy link
Contributor

@fl4via fl4via left a comment

Choose a reason for hiding this comment

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

Have you checked if there is need to update the community documentation in WF as well (in docs dir)?

@@ -79,7 +79,7 @@
static final AccessConstraintDefinition LISTENER_CONSTRAINT = new SensitiveTargetAccessConstraintDefinition(
new SensitivityClassification(SUBSYSTEM_NAME, "web-connector", false, false, false));

private static final ModelVersion CURRENT_MODEL_VERSION = ModelVersion.create(11, 0, 0);
private static final ModelVersion CURRENT_MODEL_VERSION = ModelVersion.create(12, 1, 0);
Copy link
Contributor

Choose a reason for hiding this comment

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

good catch, indeed, I missed that in the previous commit!

/**
* @author <a href="mailto:tomaz.cerar@redhat.com">Tomaz Cerar</a> (c) 2012 Red Hat Inc.
* @author Radoslav Husar
* @author Flavia Rainone
Copy link
Contributor

Choose a reason for hiding this comment

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

You can add your name here, Bartosz, so people can easily see that you created this one

@@ -125,13 +126,23 @@ private static void registerTransformersWildFly18(ResourceTransformationDescript
.getAttributeBuilder()
.setValueConverter(AttributeConverter.DEFAULT_VALUE, CONNECTION_IDLE_TIMEOUT)
.end();
subsystemBuilder.addChildResource(UndertowExtension.SERVER_PATH)
Copy link
Contributor

Choose a reason for hiding this comment

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

I could be wrong, but I think transformation is applied in a chain when comparing to an old version, and if that's the case it is enough to add a discard to registerTransformersWildFly25, a new method, in the same way I did for OBFUSCATE_SESSION_ROUTE, have you checked it the tests pass that way?
In other words, registerTransformersWildFly25 would be applied to WF25 and all previous versions

.rejectChildResource(ConsoleAccessLogDefinition.INSTANCE.getPathElement());

Copy link
Contributor

Choose a reason for hiding this comment

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

The same applies here to registerTranformersWildFly16, these changes might not be needed

@@ -155,6 +166,12 @@ private static void registerTransformers_EAP_7_2_0(ResourceTransformationDescrip
.setDiscard(DiscardAttributeChecker.DEFAULT_VALUE, PRESERVE_PATH_ON_FORWARD)
.addRejectCheck(RejectAttributeChecker.DEFINED, PRESERVE_PATH_ON_FORWARD)
.end();
final ResourceTransformationDescriptionBuilder serverBuilder = subsystemBuilder.addChildResource(UndertowExtension.SERVER_PATH);

final AttributeTransformationDescriptionBuilder ajp = serverBuilder.addChildResource(UndertowExtension.AJP_LISTENER_PATH).getAttributeBuilder()
Copy link
Contributor

Choose a reason for hiding this comment

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

The same goes for the EAP. I think we need to add a registerTransformers_EAP_7_4_0 and discard it. I'm not sure the reject has any effect, after discarding it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Im almost sure it had to be done, buy it might have been due to wrong current model. I will double check.

/**
* Test for UndertowSubsystem with subsystem schema version 12.0.
*
* @author Flavia Rainone
Copy link
Contributor

Choose a reason for hiding this comment

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

Add your name to the author list

</xs:annotation>
</xs:attribute>
<xs:attribute name="max-ajp-packet-size" type="xs:int"/>
<xs:attribute name="allowed-request-attributes-pattern" use="optional" type="xs:string"/>
Copy link
Contributor

Choose a reason for hiding this comment

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

please, add a CDATA documenting it here as well

;

/**
* The current namespace version.
*/
public static final Namespace CURRENT = UNDERTOW_12_0;
public static final Namespace CURRENT = UNDERTOW_12_1;
Copy link
Contributor

Choose a reason for hiding this comment

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

Historically we have been incrementing only the major version number for quite a while. It is intersting that you brought up this, though, because I see that Undertow has 1.1, 1.2 and 3.1. @bstansberry do we have any policies in WF as to when should we upgrade the major model version versus the minor one?

Copy link
Contributor

Choose a reason for hiding this comment

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

@fl4via @baranowb We should always do a major bump in WildFly main / WildFly Core main. That eliminates varying interpretations of what's 'minor' and leaves minor versions available for use in other streams if necessary.

I've never gotten a sense that people are trying to get semantic meaning from our API/schema versions so there's not much downside to just doing majors.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bstansberry roger.

Copy link
Contributor

@darranl darranl left a comment

Choose a reason for hiding this comment

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

For changes like this it is clearer both now and in the future to separate the version bump into it's own separate commit, when the two are combined like this it is not possible to use the GitHub UI or git diff tools later to identify the changes made to the schema.

@darranl darranl added the missing-reqs Features missing one or more of the pre-merge requirements label Nov 1, 2021
@darranl
Copy link
Contributor

darranl commented Nov 1, 2021

Added "missing_reqs" as the requirements for a feature request have not been met.

Also related to my comment about the schema version etc.. I see you have a second PR also duplicating the changes - this is another reason to use a separate commit for the bump - both RFEs can then continue from the same commit reducing the scope for conflict.

@baranowb
Copy link
Contributor Author

baranowb commented Nov 2, 2021

Have you checked if there is need to update the community documentation in WF as well (in docs dir)?

Yes. AFAIR there is no documentation that covers atttribs now, its all generated from model and descriptor files as "model reference.

@baranowb baranowb force-pushed the WFLY-15452 branch 2 times, most recently from 44571b8 to f1c5113 Compare November 2, 2021 10:25
@bstansberry bstansberry added the Feature PR provides a new feature label Nov 5, 2021
@baranowb baranowb force-pushed the WFLY-15452 branch 2 times, most recently from fb63e03 to 68f05b7 Compare November 17, 2021 10:49
@darranl darranl added the rebase-this PR has a merge conflict. label Jan 31, 2022
@fl4via
Copy link
Contributor

fl4via commented Mar 4, 2022

Have you checked if there is need to update the community documentation in WF as well (in docs dir)?

Yes. AFAIR there is no documentation that covers atttribs now, its all generated from model and descriptor files as "model reference.

In fact, you're right.

@wildfly-ci
Copy link

Hello, baranowb. I'm waiting for one of the admins to verify this patch with /ok-to-test in a comment.

@wildfly-bot wildfly-bot bot requested a review from fl4via November 9, 2023 13:00
@baranowb baranowb removed the rebase-this PR has a merge conflict. label Nov 9, 2023
@baranowb
Copy link
Contributor Author

baranowb commented Nov 9, 2023

Removed label after rebase.

@baranowb baranowb requested a review from kabir November 9, 2023 13:06
@kabir kabir removed their request for review November 9, 2023 13:20
@kabir kabir dismissed their stale review November 9, 2023 13:20

Added by mistake

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deps-ok Dependencies have been checked, and there are no significant changes Feature PR provides a new feature missing-reqs Features missing one or more of the pre-merge requirements
Projects
None yet
6 participants