-
Notifications
You must be signed in to change notification settings - Fork 420
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
Support multiple certificate and different number of empty lines for netsh #1153
base: master
Are you sure you want to change the base?
Conversation
…t of netsh. Also support if there is more than one certificate. fix microsoft#919
@microsoft-github-policy-service agree |
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.
@LeftTwixWand @starkmsu @manolerazvan 👋 I hate to bug you guys, but can we merge this crucial fix? Depending on your version of Windows it will cause IIS deployment issues due to different netsh outputs based on its version. Windows Server 2012 - netsh.exe @ 6.3.9600.17415 works with this extension ✔️ Thank you! |
Related Issues: Duplicated Pull Requests: |
Hello Preston and @yepeekai! Thank you so much for your efforts, it's great to see when community takes over some issues. I'll keep you up to date😊 |
Generally, PR looks good, I will just add a one more test here: TaskModules/powershell/Tests to cover the new functionality. |
I just noticed this line will need to be updated to find by key to get the certificate hash - Line 209 in dd5e1b9
The same fixes also need to be applied for IISWebAppDeployment - https://github.com/microsoft/azure-pipelines-tasks/blob/065dfd9c37ff3e0bae7bd385bfa523fc17f98407/Tasks/IISWebAppDeployment/MsDeployOnTargetMachines.ps1#L277-L298 |
I committed a change to validate the certificate hash. I also changed the binding search to ensure there is no false positive. For example if we search for port 443 and there is a certificate for 443 and 44311, both would have matched in the previous version and would have cause problem. |
I tested the refactored method, and everything looks good. I'll try to test these changes in the real environment. |
Would be great to get this MR merged in the near future...We are running into this issue aswell |
@LeftTwixWand I can confirm this is the wrong file modified! 🤦 I wonder what the history is for why IIS Tasks didn't inherit from this other AppCmdOnTargetMachines.ps1. I'm a big fan of this the new functions introduced in this PR. Additional related PRs that can likely be closed once this is merged: |
That's sad. Maybe still begin with this PR and then replicate the changes over to the other place |
Hey @yepeekai @coolhome @CJohannimloh I know that this issue is taken buy few colleagues from my team. Let me ask them about the progress. |
Hey @yepeekai @coolhome @CJohannimloh can some of you provide the next information and I'll try to complete this PR ASAP:
This would help me a lot. Because I still have other high priority work to do. |
Hey @LeftTwixWand, sure i can give some information.
How to reproduce:
|
Hey @CJohannimloh thank you so much for your help. We're already working on it. |
@LeftTwixWand in addition this is the area we forget. I'm using the same as above, but you need to enter in the remote host information. The one above was for Deployment Groups where this is using WinRM to deploy. - task: IISWebAppMgmt@3
inputs:
machinesList: '$(RemoteHostname)'
AdminUserName: '$(RemoteUser)'
AdminPassword: '$(RemotePass)'
WinRMProtocol: Http
WebsiteName: 'testwebsite'
WebsitePhysicalPath: '%SystemDrive%\inetpub\wwwroot\testwebsite'
AddBinding: true
CreateOrUpdateAppPoolForWebsite: true
ConfigureAuthenticationForWebsite: true
Protocol: https
Port: 443
ServerNameIndication: false
SSLCertThumbPrint: '$(Thumbprint)'
AppPoolNameForWebsite: 'testapppoool'
AnonymousAuthenticationForWebsite: true
WindowsAuthenticationForWebsite: false |
Hey @yepeekai and @coolhome may I ask a question about this code changes. Because when I go to the task folder on the build agent I can see this: |
My understanding of azure tasks stuff is limited... LeftTwixWand made a comment in that regard on july 26th if you go up in the history of this PR. It in fact appears that I did modify at the wrong place, this peace of code is duplicated and I choose the wrong one when I searched for specific patterns. On my side I am not able to test azure directly with this, so I copied the part that I though was used and executed it on my machine (and a dev server we have). I suggested to correct it here and then port the solution to the other place. |
@yepeekai thank you so much for this response. Anyway, thank you for this pr, I'm sure it will be merged soon. |
I have somewhat identical setup, one The first run everything is setup just fine, all following runs results in.
Is this because of the same issues mentioned in this thread? I have something more like
|
Running into the same issue with the same setup as @jonascarlbaum First time always works, second time get the error
Any update on this @LeftTwixWand |
Yep, this is really bad @scsloan. Did a workaround, can’t copy on my phone right now, but can screenshot the outline of the task I run prior this one now. The missing part on bottom line is |
I feel this workaround is not a valid solution in the long run… |
Thanks for sharing your workaround @jonascarlbaum. |
No problems @scsloan! But I would love a permanent fix! @LeftTwixWand is these issues going to be addressed and fixed any time soon? |
Why hasn't this PR been merged? I'm having this problem and would really appreciate to have it fixed. |
Think the fix isn’t in correct files. But I had hopes someone had solves this after such long time… Since the issue is easily reproducable it would be something that just should be solved. More and more people will get trouble using these tasks… |
Yes. Frankly, how do we request priority on these issues? The task seems to have been abandoned 5 years ago - no new commits for five years!? But as far as I know the deployment group/agent approach for DevOps release pipelines hasn't been deprecated, so the task needs to keep up. |
@coolhome @LeftTwixWand any update on this? |
Any updates on this topic? |
@CJohannimloh I haven't heard anything back. So instead we are looking at just disable |
@colhouse-hakh: Ye sure. We are doing the same but still want a better solution |
Hi @colhouse-hakh @CJohannimloh, |
What exactly do you need? |
We need pipeline logs as similar fix we did for the issue but still many of you are still having some issues. |
Of course we can’t mess with current production environments. It’s easier doing such things during the time you build the pipeline. But the only relevant part of the logs seems still being the part
|
@v-schhabra
The error is described in more details in issue #1008 has all the details you require for this ticket. This ticket was also mentioned by @coolhome in this PR on 23rd May 2023. |
Support multiple certificate and different number of empty lines for netsh. fix #919